-
Notifications
You must be signed in to change notification settings - Fork 31
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
time extension via SequencePT #830
base: master
Are you sure you want to change the base?
Conversation
main into feat/time_extension_pulse_template for testing
Test Results 6 files ± 0 6 suites ±0 5m 1s ⏱️ + 1m 25s For more details on these failures, see this check. Results for commit db9f56c. ± Comparison against base commit 1b3838e. ♻️ This comment has been updated with latest results. |
I am not anymore convinced that encoding this in the type has enough advantages. A class method on sequencept could do the same. |
The main point of increased convenience would probably be the automated insertion into the program creation by overwriting the |
self.assertEqual(duration_extended, duration_summed) | ||
|
||
def test_pad_to_usecase(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean by the previous comment is basically this, which gives a - in my opinion - quite convenient way of quickly extend waveforms with shorter and longer hold times guaranteed to match the hardware requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a utility function like this would be sufficient
def extend(self, *, total_duration = None, prepend = None, append = None) -> SequencePT:
none_count = total_duration is None + prepend is None + append is None
if none_count != 1:
raise ValueError("Exactly two arguments must be specified.")
# Return a SequencePT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for that case of not concatenating the waveforms, yes; what about the single wf case? (Which i initially added this for, the other one was then mostly to have similar syntax)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but why the SequencePT? Why represent the start and end as PulseTemplates? I would either have a PT that represents the time extention without introducing ConstantPTs in the front and in the back OR use SequencePT and add an atomic flag to it which encodes potential atomicity in the PT itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seemed like the least programmatic effort to me that does the job, but that can of course be changed.
inner_program = inner_builder.to_program() | ||
|
||
if inner_program is not None: | ||
#measurements not yet included |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linspace builder ignores measurements. The idea is to have a dedicated MeasurementBuilder
to extract them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
somewhat against the design principles discussed in #824, but maybe somewhat convenient for
pad_to
related utility?branch https://github.com/qutech/qupulse/tree/feat/time_extension_pulse_template is more elaborate, however, in which situation is it really needed to deviate from a sequencePT-style?