-
Notifications
You must be signed in to change notification settings - Fork 65
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
Conversation
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.
Here is a first, non-exhaustive review.
A general issue I have with this approach is its "hard-codedness"... We manually encode these fields to compare, which forces us to remember to add new fields whenever we make changes to the classes.
I would be in favour of a more robust solution where this happens automatically or, if that's not possible, at least where these parameters live closer to the classes so that we remember to change them.
) | ||
|
||
|
||
def validate_channel_from_best( |
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
and DMM
.
"addressing": ne, | ||
"max_abs_detuning": gt, | ||
"max_amp": gt, | ||
"min_retarget_interval": lt, | ||
"fixed_retarget_t": lt, | ||
"max_targets": gt, | ||
"clock_period": lt, | ||
"min_duration": lt, | ||
"max_duration": gt, | ||
"mod_bandwidth": gt, |
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 when strict=True
) - where, by the way, I see these functions fitting very naturally
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 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:
if emulator is None:
available_devices = self.fetch_available_devices()
# TODO: Could be better to check if the devices are
# compatible, even if not exactly equal
if sequence.device not in available_devices.values():
raise ValueError(
"The device used in the sequence does not match any "
"of the devices currently available through the remote "
"connection."
)
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
:
- takes into account whether the channels are reusable or not when matching channels of the devices.
basis
should be equal.- if
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
). - if
strict
,fixed_retarget_t
should match exactly. Given its definition "Time taken to change the target (in ns)." that's correct. - if
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
. - tests all the other properties of device by implementing the sequence with it.
- Misses the comparison of layout/register of the Sequence with the calibrated_layouts of the new device (if there is a matching layout, the register of the sequence should have this new layout)
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 in Sequence.switch_device
, the default operator is ne
, whereas in the validation functions they are None
, 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/eom ne
, 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.
- if
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
).
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...
eom_att = asdict(eom) | ||
|
||
# Error if attributes in eom and best_eom compare to True | ||
comparison_ops = {"mod_bandwidth": gt} |
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
"limiting_beam": ne, | ||
"max_limiting_amp": gt, | ||
"intermediate_detuning": ne, | ||
"controlled_beams": gt, |
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
Closes #557