Unintuitive taping behaviour about pyadjoint

[dham]

I think that's right. It basically comes down to what we think the semantics of the three operations are. The solution above works if we think the semantics are:

pause_anotation() - unconditionally switch off annotation.

continue_annotation() - unconditionally switch on annotation.

stop_annotation() - cease annotation inside the context manager and return to the previous annotation state afterwards.

I confess that that doesn't quite line up with the names "pause" and "continue", but I think that that's how those functions are actually used. I also don't think there is a sane way of having "pause" and "continue" have state. I think the current mess probably results from an attempt to have them do so.

As an aside, even this isn't really safe in the context of multiple tapes, but I think that is something which should be fixed elsewhere (basically, the annotation state should be stored on the tape and switching tapes should have the corresponding effect on the annotation state).

[dham]

This isn't my code, but I think that what is going on here is an attempt to make the following code work correctly:

from pyadjoint.tape import continue_annotation, annotate_tape, stop_annotating

pause_annotation()
with stop_annotating():
    pass
 assert not annotate_tape()  # raises an error

This explains why a plain boolean isn't sufficient. However, what is currently being done is abusing an integer as an incorrectly implemented stack. It would be better to have an actual stack. The easiest way to implement this would probably be to have the stop_annotation context manager keep the state of annotation before it executes and restore that.

[connorjward]

This explains why a plain boolean isn't sufficient. However, what is currently being done is abusing an integer as an incorrectly implemented stack. It would be better to have an actual stack. The easiest way to implement this would probably be to have the stop_annotation context manager keep the state of annotation before it executes and restore that.

I can see how something like

class stop_annotating:
    def __enter__(self):
        global _annotation_enabled
        _annotation_enabled = False

    def __exit__(self, *args):
        global _annotation_enabled
        _annotation_enabled = True  # not always the right thing to do

would be wrong. Are you suggesting instead that everything would work if we had something like

class stop_annotating:
    def __enter__(self):
        global _annotation_enabled
        self._orig_annotation_enabled = _annotation_enabled
        _annotation_enabled = False

    def __exit__(self, *args):
        global _annotation_enabled
        _annotation_enabled = self._orig_annotation_enabled

I just want to be clear that when you say "have an actual stack" you don't mean to use a list or similar to pop and push from. The state awareness of the context manager is sufficient.