Is the old instructions method being deprecated? Is it specifically for ScheduleBlocks? This looks like a breaking change to me

This is only for ScheduleBlock since it doesn't have t0.

This change would break user code, so it's a breaking interface change. Whenever possible, we should avoid this.

Perhaps for ScheduleBlock.instructions, we should try to convert to a Schedule, or print a warning, or error and ask users to use a different method. I'm not sure what the best, but we shouldn't expect them to make this change without some assistance. Do you have any ideas?

I agree we want to avoid breaking changes for existing users when possible, which this likely is since we swapped out Schedule for ScheduleBlock. Ideally, we convert to a Schedule as @lcapelluto describes and warn that this method will be deprecated in favour of timed_instructions in a future PR and use another property (what should this be named?) instead that does not raise this deprecation warning for this behavior. We could always reintroduce instructions as an attribute after the deprecation period.

Thanks. I updated the behavior of ScheduleBlock.instructions. Now it converts self into Schedule and call .instructions. The original method is renamed to ScheduleBlock.blocks. I don't think this change has impact to the user code. I'm not sure if we need to rename instructions to timed_instructions, since this causes tons of deprecation warnings in tutorials.

Couldn't a schedule be trivially made into a ScheduleBlock? (This looks like a breaking change. If it's not, then it's safe to ignore this comment)

Good question. The docstring of pulse.build says

        schedule: A *mutable* pulse ``ScheduleBlock`` in which your pulse program will
            be built.

I guess this indicates

my_sched1 = Schedule()
with build(schedule=my_sched1):
    # do something

my_sched2 = Schedule()
with build() new_sched:
    # do something
my_sched1 == my_sched2.append(new_sched)

So the builder should operate on the input schedule in the mutable fashion. However we cannot append ScheduleBlock to Schedule because of the ambiguity of duration. Thus this will become breaking change. If there is any approach to avoid this, I'm happy to update the logic.

This is what worked before:

with build(schedule=Schedule(...)) as new_schedule:
   ...

it looks like this would error now. My suggestion is to do something like:

if isinstance(block, Schedule):
    tmp = ScheduleBlock()
    tmp.append(block)  # I forget what the interface is here. 'call'?
    block = tmp

Okey, so it seems like the docstring is not correct. block is not necessary to be mutable?

The "and append to parent" seems uncommon for pop. Is there another term we can use for this?

This made compile below very difficult to understand:

        while len(self._context_stack) > 1:
            self.pop_context()

without reading the implementation of pop_context, it looks like this would just remove everything.

Umm you're right. Any suggestion? pop_append_context?

Yes, that's definitely better. 🤔 Not sure I have a better suggestion. process_last? @eggerdj, ideas?

I lean toward updating the behavior of method. push & pop are standard terms for stack data structure and stick to these names would be better for developers. Instead of internally appending to parent, we can call builder.append_block method externally.