You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
If annotations are switched off and you use the set_working_tape context manager then nothing gets taped, which can be confusing and/or inconvenient in some circumstances (e.g. if you are inside some function/library and annotation was switched off above you in the stack).
We're also trying to move to set_working_tape being the best-practice to limit the scope of each tape, so you end up writing things like:
This PR makes set_working_tape do the logic to a) make sure we have annotations on inside the context manager and b) return the annotation state to the value before the context manager. The above code just becomes:
withset_working_tape() astape:
...
The previous behaviour can be obtained by setting the continue_annotations kwarg
Is this the right approach? I would expect that if I have some code like
withstop_annotating():
do_something()
Then taping should always be disabled in do_something().
You can get that with set_working_tape(continue_annotation=False). Setting the default to True is a somewhat opinionated personal preference, I wouldn't be totally averse to changing the default to False to avoid this being an API breaking change.
I'd also say that stop_annotating arguably is implicitly saying "stop annotating the current working tape", so it is questionable if it should always be propogated if the tape changes.
I feel like it makes sense to decouple the notions of which tape is the active one and whether taping is enabled, but that's just me.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If annotations are switched off and you use the
set_working_tapecontext manager then nothing gets taped, which can be confusing and/or inconvenient in some circumstances (e.g. if you are inside some function/library and annotation was switched off above you in the stack).We're also trying to move to
set_working_tapebeing the best-practice to limit the scope of each tape, so you end up writing things like:This PR makes
set_working_tapedo the logic to a) make sure we have annotations on inside the context manager and b) return the annotation state to the value before the context manager. The above code just becomes:The previous behaviour can be obtained by setting the
continue_annotationskwarg