Report offending input in sequence module error messages - #1098
Report offending input in sequence module error messages#1098jaewonyun1234 wants to merge 4 commits into
Conversation
602328d to
0dff57e
Compare
HGSilveri
left a comment
There was a problem hiding this comment.
Nice going @jaewonyun1234 !
Here are a few minor comments, keep up the good work :)
| for q in self._register.qubit_ids | ||
| if q in last.targets |
There was a problem hiding this comment.
You can just iterate over the last.targets here
There was a problem hiding this comment.
last.targets is a set[QubitId], so iterating it directly builds the dict in hash order, which changes from run to run. Going through self._register.qubit_ids and filtering is what keeps the printed order stable.
With your version, test_phase fails on 7 of 8 PYTHONHASHSEED values. On the 28-qubit fixture register the same error prints as:
seed 0: got {'q9': 0.0, 'q0': 1.0, 'q7': 0.0, 'q15': 0.0, ...}
seed 1: got {'q17': 0.0, 'q9': 0.0, 'q11': 0.0, ..., 'q0': 1.0, ...}
q0 is the only qubit with a different reference, and it moves around each run.
That said, you're right that the loop doesn't show why it's written that way. Would you prefer a small helper, used here and at the two other spots you flagged, plus the same pattern in config_slm_mask?
def _in_register_order(self, qubits: Collection[QubitId]) -> list[QubitId]:
"""Qubit ids in the register's order, so error messages are reproducible."""
return [q for q in self._register.qubit_ids if q in qubits]Separately on this message: on a global channel it lists every qubit, so 27 of the 28 entries are the same 0.0 and only q0 differs. Would you rather it grouped them by reference value, or showed only the ones that stand out?
There was a problem hiding this comment.
Or, if a new method feels like too much for this, I can just add a one-line comment at each spot instead:
# Iterate the register, not the target set, so the order is reproducibleHappy either way — whichever you prefer.
There was a problem hiding this comment.
How about iterating over sorted(last.targets)?
| for q in self._register.qubit_ids | ||
| if q in last.targets |
There was a problem hiding this comment.
Same reason as the thread above — these are sets, so the register loop is what keeps the printed order stable. Happy to switch all three to a helper if you prefer that.
| for q in self._register.qubit_ids | ||
| if q in qubit_ids_set |
There was a problem hiding this comment.
Same reason as the thread above — these are sets, so the register loop is what keeps the printed order stable. Happy to switch all three to a helper if you prefer that.
Error messages that only stated a rule now also report the value that broke it, so the user can see what was wrong without reproducing the failure in a debugger. Covers 38 messages in pulser/sequence: 35 in sequence.py, 2 in _schedule.py and 1 in _seq_drawer.py. Messages describing a state rather than a rejected input are left unchanged, since there is no offending value to report. Existing assertions that stopped before the reported value are extended to the end of the message, and a missing test is added for the DMM sample extraction error.
Use an instance of for the BaseDevice isinstance check. Report still-available DMM ids when a DMM cannot be declared, and report the given targets when a local channel exceeds max_targets.
The stored-call path now reports the given targets, so the old substring no longer matches.
Follows the review on pasqal-io#1098: - magnetic_field says "this sequence addresses" rather than "uses", to match get_addressed_bases(). - The SLM mask DMM error now says what to do next instead of listing state: "before adding a pulse to 'dmm_0', make sure at least one of ['ryd'] already has a pulse." - _ChannelSchedule.__getitem__ no longer mentions time slots, which are an internal object. - The qubit-content drawing error names the 'ground-rydberg' basis instead of the vaguer "rydberg basis". Three of the suggested lines needed a small correction to work as written: a missing "is", a missing f-prefix that would have printed "{self.channel_id!r}" literally, and a double space.
0dff57e to
2d746d2
Compare
Hi @a-corni, @HGSilveri,
Following on from #1095 and #1097, this covers the sequence module (38 messages: 35 in
sequence.py, 2 in_schedule.py, 1 in_seq_drawer.py).Error messages that only stated a rule now also report the value that broke it.
_schedule.pyreports the channel with no target and the duration that overflowed;_seq_drawer.pyreports the channels that aren't in the rydberg basis. Qubit-id errors still list the declared ids — those sets are small and useful to see.A few were more than wording:
'Microwave'clash indeclare_channelinterpolated the channel object, so it printed the full spec (~236 characters) instead of the name the user chose. It now reports the name and id, e.g.got 'ryd' ('rydberg_global') with the declared {'mw': 'mw_global'}.estimate_added_delayhad two adjacent string literals with no space, so it read...if sequence orpulse is parametrized._schedule.get_samplesraises whenqubitsis missing for a DMM channel, but no test reached it. Added one.Also aligned with the register review:
'device' must be an instance of 'BaseDevice'; an unavailable DMM lists the still-available ids; a local channel that exceedsmax_targetsreports the limit, the count, and the given targets.Messages that describe a state rather than a rejected input are left unchanged (
_in_isingtransitions,qubit_info,register,get_measurement_basis, SLM on a mappable register,_targetwith no qubits, empty-sequence drawing).Existing
match=assertions now cover the full message (re.escapewhere needed). Partially addresses #1057.