Report offending input in channels module error messages - #1102
Open
jaewonyun1234 wants to merge 1 commit into
Open
Report offending input in channels module error messages#1102jaewonyun1234 wants to merge 1 commit into
jaewonyun1234 wants to merge 1 commit into
Conversation
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 11 messages in pulser/channels: 7 in base_channel.py, 2 in eom.py, 1 in dmm.py and 1 in modulation.py. Also fixes the beam validation in RydbergEOM, which reported self.limiting_beam instead of the beam being checked. An invalid entry in controlled_beams was therefore reported as the limiting beam, which is usually valid. The three validate_pulse limits now share one shape: the rule, the limit in parentheses, then the value. validate_duration no longer builds its message with %-formatting, and the mod_bandwidth upper bound gains its missing full stop. 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 the beam validation test now checks the reported beam.
a-corni
self-requested a review
September 8, 2026 10:19
a-corni
reviewed
Sep 8, 2026
a-corni
left a comment
a-corni
left a comment
Member
There was a problem hiding this comment.
Hey @jaewonyun1234 ! Thanks for your contribution! Here are some suggestions :)
| raise TypeError( | ||
| "Every beam must be one of options of the `RydbergBeam`" | ||
| f" enumeration, not {self.limiting_beam}." | ||
| f" enumeration, not {beam}." |
Member
There was a problem hiding this comment.
Nice catch! Actually, I think it would be more interesting to say which attribute has an incorrect type. Perhaps we could use a dict instead of a chain, having {"limiting_beam":self.limiting_beam, "controlled_beams":self.controlled_beams} and
Suggested change
| f" enumeration, not {beam}." | |
| f" enumeration. Got {beam} for attribute {dict_key}." |
Comment on lines
+488
to
+490
| "The pulse's amplitude goes over the maximum value allowed " | ||
| f"for the chosen channel ({self.max_amp}); got " | ||
| f"{amp_samples_np.max()}." |
Member
There was a problem hiding this comment.
I think we should specify the pulse that generated this error.
Suggested change
| "The pulse's amplitude goes over the maximum value allowed " | |
| f"for the chosen channel ({self.max_amp}); got " | |
| f"{amp_samples_np.max()}." | |
| "The pulse's amplitude goes over the maximum value allowed " | |
| f"for the chosen channel ({self.max_amp}); got " | |
| f"a maximum amplitude {amp_samples_np.max()} in pulse {pulse!r}." |
| raise ValueError( | ||
| "The pulse's detuning values go out of the range allowed " | ||
| f"for the chosen channel ({self.max_abs_detuning}); got " | ||
| f"a maximum absolute value of {abs_detuning.max()}." |
Member
There was a problem hiding this comment.
Likewise, it is important to return the pulse in this error message
Suggested change
| f"a maximum absolute value of {abs_detuning.max()}." | |
| f"a maximum absolute detuning of {abs_detuning.max()} in pulse {pulse!r}." |
| raise ValueError( | ||
| "The pulse's average amplitude is below the chosen " | ||
| f"channel's limit ({self.min_avg_amp})." | ||
| f"channel's limit ({self.min_avg_amp}); got {avg_amp}." |
Member
There was a problem hiding this comment.
Likewise, it is important to return the pulse here
Suggested change
| f"channel's limit ({self.min_avg_amp}); got {avg_amp}." | |
| f"channel's limit ({self.min_avg_amp}); got average amplitude {avg_amp} in pulse {pulse!r}." |
| raise ValueError("The detuning in a DMM must not be positive.") | ||
| raise ValueError( | ||
| "The detuning in a DMM must not be positive; got a maximum " | ||
| f"of {round_detuning.max()}." |
Member
There was a problem hiding this comment.
I believe what matters here is the detuning of the pulse (maybe not necessarily the pulse, as it's a DMM)
Suggested change
| f"of {round_detuning.max()}." | |
| f"of {round_detuning.max()} in detuning {pulse.detuning!r}." |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Hi @a-corni, @HGSilveri,
Following on from #1095, #1097 and #1098, this covers the channels module: 11 messages across
base_channel.py(7),eom.py(2),dmm.py(1) andmodulation.py(1).One was a bug rather than wording.
RydbergEOMchecks each beam inchain((self.limiting_beam,), self.controlled_beams), but the message printedself.limiting_beaminstead ofbeam. A bad value incontrolled_beamswas reported as the limiting beam, which is usually valid:The existing assertion stopped before that value, so it passed either way. It now checks the beam.
Partially addresses #1057.