Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScheduleBlock 4/4 - add ScheduleBlock #6158

Merged
merged 43 commits into from
Apr 24, 2021

Conversation

nkanazawa1989
Copy link
Contributor

Summary

This is 4/4 step of ScheduleBlock implementation #5679

Details and comments

This PR mainly aiming at replacing the output program of the builder context with ScheduleBlock. The builder is now managed with stack data structure (FILO) to leverage the block representation.

Since chained transformation appears everywhere, I added qiskit/pulse/transforms/base_transforms.py to define typical chain of transformation to generate an executable schedule. This will be replaced with proper transformation passes #6121 .

In this PR unittests of the builder are temporarily performed by converting the output programs into schedule with above transformation (note that reference programs are currently written in Schedule representation). This may hurts CI performance, so these unittests will be replaced shortly #6106 .

Some builder functions are deprecated

  • pulse.pad: No longer necessary and sometime requires quite complicated logic. For example,
with build() as sched:
    with align_equispaced(duration=1000):
        play(my_pulse, d0)
        with pad():
            play(my_pulse, d0)
            play(my_pulse, d0)
            play(my_pulse, d0)
        play(my_pulse, d0)

here the nested pad context is first evaluated, but delay in between pulses cannot be determined until the outer context (equispace) is evaluated. In this PR, pad is move to transformation passes that is applied to the entire schedule.

  • pulse.frequency_offset(compensate_phase=True): In the same reason, this requires complicated logic to evaluate duration of nested context. Currently no unittest for this situation exists. Instead, we can use signal formalism Support frames using signals #5977 -- this will calculate phase accumulation after all pulse locations are resolved.

  • pulse.inline: I cannot find any practical use case for this though the implementation requires recursive instruction call for nested context (i.e. complicated and not good performance). Instead, we can just manually remove all alignment contexts within the inline context.

- update builder logic
  - deprecate call_schedule
  - deprecate pad context
- remove auto padding of align_equispaced and align_func
- move pad from alignment to canonicalization
- add temp transform pass
- unittest fix
@nkanazawa1989 nkanazawa1989 added Changelog: API Change Include in the "Changed" section of the changelog mod: pulse Related to the Pulse module labels Apr 5, 2021
@nkanazawa1989 nkanazawa1989 added this to the 0.18 milestone Apr 5, 2021
Copy link
Contributor

@lcapelluto lcapelluto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review IP. went through all the small files, now on to the main one: builder.py

qiskit/scheduler/lowering.py Outdated Show resolved Hide resolved
qiskit/visualization/pulse_v2/core.py Outdated Show resolved Hide resolved
test/python/pulse/test_block.py Show resolved Hide resolved
test/python/pulse/test_builder.py Show resolved Hide resolved
self.assertEqual(cal_sched.instructions[0][1].frequency, freq - delta + beta)
self.assertEqual(cal_sched.instructions[0].frequency, freq - delta + beta)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@lcapelluto lcapelluto Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@taalexander taalexander Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

qiskit/pulse/transforms/canonicalization.py Show resolved Hide resolved
Comment on lines 331 to 333
if not isinstance(block, ScheduleBlock):
raise exceptions.PulseError(f'Input program {repr(block)} is not `ScheduleBlock`. '
'This cannot be a root context.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

qiskit/pulse/builder.py Outdated Show resolved Hide resolved
qiskit/pulse/builder.py Outdated Show resolved Hide resolved

@_compile_lazy_circuit_before
def pop_context(self):
"""Pop the last context from the stack and append it to the parent."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm you're right. Any suggestion? pop_append_context?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nkanazawa1989
Copy link
Contributor Author

@peendebak Thanks for testing the PR! I completely have forgotten this type checking in the instmap. The instmap is updated to allow the block programs. These blocks are internally converted into conventional schedule just before the job submission anyway, so you can use instmap without considering program types.

@taalexander Thanks for the review! The PR is updated ;)

@peendebak
Copy link
Contributor

@nkanazawa1989 Thanks for the quick response. Adding to the inst_map works now, but scheduling still fails. A minimal example:

from qiskit import QuantumCircuit, schedule
from qiskit.test.mock import FakeAlmaden
from qiskit import pulse
from qiskit.circuit import Gate
from qiskit.pulse import DriveChannel

def build_schedule(duration, amplitude, frequency_offset, channel):
    with pulse.build() as pulse_prog:
            with pulse.frequency_offset(frequency_offset, channel):
                    pulse.play(pulse.library.Constant(duration=duration, amp=amplitude, name='genericX'), channel)
    return pulse_prog

myschedule = build_schedule(10, .1, -.3, DriveChannel(0))


qc = QuantumCircuit(2)
gate =Gate('mygate', 1, [])
qc.append(gate, [0])
print(qc.draw())

backend = FakeAlmaden()
defaults = backend.defaults()
inst_map = defaults.instruction_schedule_map

inst_map.add('mygate', [0], myschedule) # works now
pulse_schedule = schedule(qc, backend) # does not work yet

@nkanazawa1989
Copy link
Contributor Author

nkanazawa1989 commented Apr 22, 2021

Okey, I found the problem, however I feel it would be better to rewrite circuit scheduler to be based on the block representation, i.e. we can keep duration parametrized in the mapper. Could you please open an issue for the instmap after this PR is merged? This upgrade will be relatively major change, so I think it's better to separate PR.

For now, you can schedule your circuit by using pulse gate:

from qiskit import QuantumCircuit, schedule
from qiskit.test.mock import FakeAlmaden
from qiskit import pulse
from qiskit.circuit import Gate
from qiskit.pulse import DriveChannel

def build_schedule(duration, amplitude, frequency_offset, channel):
    with pulse.build() as pulse_prog:
            with pulse.frequency_offset(frequency_offset, channel):
                    pulse.play(pulse.library.Constant(duration=duration, amp=amplitude, name='genericX'), channel)
    return pulse_prog

myschedule = build_schedule(10, .1, -.3, DriveChannel(0))


qc = QuantumCircuit(2)
gate =Gate('mygate', 1, [])
qc.append(gate, [0])
qc.add_calibration(gate, (0,), schedule=myschedule)
print(qc.draw())

backend = FakeAlmaden()
defaults = backend.defaults()
pulse_schedule = schedule(qc, backend)

taalexander
taalexander previously approved these changes Apr 22, 2021
Copy link
Contributor

@taalexander taalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mergify mergify bot merged commit c65b41f into Qiskit:main Apr 24, 2021
@peendebak
Copy link
Contributor

@nkanazawa1989 Thanks for fixing my issue. Is it still needed to open a bug report? I am not sure what to report, since the actual problem is not occuring any more.

@nkanazawa1989
Copy link
Contributor Author

This fix is temporarily (block is converted forcibly into schedule now), so I plan to make another PR to keep blocks as-is. However if this fix solved your issue, you don't need to write an another issue. Thanks for finding and reporting it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the "Changed" section of the changelog mod: pulse Related to the Pulse module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants