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.
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
Add validation of a Device/Channel/EOM from another Device/Channel/EOM, and comparison of RegisterLayouts #558
Add validation of a Device/Channel/EOM from another Device/Channel/EOM, and comparison of RegisterLayouts #558
Changes from all commits
cf81fcc
67de114
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 do you think of turning this into a method? We could have something like
Channel.is_replaceable_by(better_channel: Channel)
.The same would go for the EOM, probably.
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.
Yes that was also an idea I had, that seemed quite pleasant to me. Also pleasant because it can be redefined/ extended for
LaserChannel
andDMM
.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'm afraid things are not this simple for the quantities that affect the sequence when it is being reconstructed. For example,
mod_bandwidth
influences things like the wait times, so if you are rebuilding the sequence with a device that has a different modulation bandwidth (even if higher), it may turn out different.I suggest you take a look at how
Sequence.switch_device()
works (particularly whenstrict=True
) - where, by the way, I see these functions fitting very naturallyThere 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 now see the mistake with my approach: it completely forgets the original goal of this validation, which is to check whether sequence from the the cloud can be performed with the available_devices. As a reminder:
I only wanted to check if the values put in Device/Channel/EOM matched with Device/Channel/EOM, but that is not the topic indeed. The checks should garantee that the shape of the curve will be the same. So indeed the mod bandwidth should be the same. The clock_period should be the same as well (or a divider of the clock period of the best channel).
Here is a list of things that are different in
Sequence.switch_device
:basis
should be equal.strict
, the eom configs have to be exactly the same. To me, that's right for all the attributes except for controlled_beams: we can make a list of the controlled beams config ( RED, BLUE, (BLUE, RED) ) used in each eom block of the sequence, and check if they are among the controlled beams of the new channel (if the current device has controlled beams (RED, BLUE, (BLUE, RED)) but only the RED is controlled in the EOM blocks of the Sequence, a Channel with controlled beams (RED) would workstrictly
).strict
,fixed_retarget_t
should match exactly. Given its definition "Time taken to change the target (in ns)." that's correct.strict
,min_retarget_interval
should match exactly. As this is for testing only, and as its definition is "Minimum time required between the ends of two target instructions (in ns).", I think that can be changed intonew_channel.min_retarget_interval <= old_channel.min_retarget_interval
.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.
To add another remark, the difference between
Sequence.switch_device
and the validation functions is that inSequence.switch_device
, the default operator isne
, whereas in the validation functions they areNone
, meaning that no comparisons can be done. If I were to keep these validation functions, I would build the dictionary of comparison operators by first assigning to all the attributes of device/channel/eomne
, and then specify a different operator for some of 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.
I agree with this, the problem is checking it... The decision of which beams are controlled depends on the value of
detuning_on
, which may even be parametrized, so we can't always know in advance. And even when we can, we currently don't store the controlled beams in in each block so we have to reverse engineer it, which is a pain...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.
Should add the
custom_buffer_time
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.
Should add
multiple_beam_control