Improve information content in error messages

[HGSilveri]

A lot of error messages have been written assuming that the user would hit it "directly" (i.e. when coding in some interactive environment like a Jupyter notebook). Take for example this error:

 ValueError("All samples of an amplitude waveform must be greater than or equal to zero.")

This is fine if the user just tried to create a Pulse. However, if it happens deep inside some function in the code base, potentially coming from the building of some parametrized sequence, it can be harder to tell what is going on. An immediate improvement would be to just add more information in the message, e.g.

 ValueError(f"All samples of an amplitude waveform must be greater than or equal to zero; got negative values for waveform {amp_wf}")

This alone might help narrow down where the error is coming from.

We should do this judiciously throughout the codebase.

[Janvee31]

Hi! I'm new to contributing to Pulser and I'd like to work on improving the contextual information in error messages for this issue. If no one else is currently working on it, could this issue be assigned to me?

[a-corni]

Hello @Janvee31,
Welcome! It's a pleasure to receive new contributions! The issue is assigned to you :)

[jaewonyun1234]

Hi @HGSilveri, @a-corni — I'd like to help with this one if you're open to it.

Since #1058 tackles the Pulse side, would you be willing to assign me pulser-core/pulser/waveforms.py as a separate piece? Happy to take a different module if that fits better.

There are 7 raise sites in that file (6 distinct messages) that state a rule without reporting the offending input, e.g.:

• "The step of the slice must be None or 1." — doesn't report s.step
• "Needs at least two waveforms to form a CompositeWaveform." — doesn't report how many were given
• "`times` must be an array of non-repeating values." — doesn't report which values repeat

"The maximum value and the area must have matching signs." also appears in both BlackmanWaveform.from_max_val and KaiserWaveform.from_max_val. Since both are decorated with @parametrize, in a parametrized sequence this surfaces at build() time, where the message alone doesn't say which waveform produced it.

I'd follow the style already in the file (the duration and beta checks both interpolate the value), and append detail after the existing sentence rather than reword in place, so the match= assertions in tests/test_waveforms.py keep passing.

[a-corni]

Hello @jaewonyun1234,
It's a pleasure to hear you'd like to contribute to Pulser!
Indeed, #1058 (finalized in #1093) tackled this issue partially. Thanks @Janvee31 for your work!
I am assigning you pulser-core/pulser/waveforms.py. Side comment: I think that what matters in this work is more the input object that the precise failing property. Imagine you have a software generating the inputs to a Waveform, you'd like to know which call to a Waveform is failing, the rest of the message should be enough speaking to understand what is causing it.
Therefore:

• "The step of the slice must be None or 1." — doesn't report s.step -> should rather report the input slice imo, otherwise the software developer might never be able to know which call caused this issue. You could also have something like "Slice {s} has step {s.step}."
• "Needs at least two waveforms to form a CompositeWaveform." — doesn't report how many were given -> maybe add as well the waveform that was provided, if there was one.
• "times must be an array of non-repeating values." — doesn't report which values repeat -> good to report which values repeat, but should also report the times ("Found repeating values in times {times}: {repeating_value}")

"The maximum value and the area must have matching signs." also appears in both BlackmanWaveform.from_max_val and KaiserWaveform.from_max_val. Since both are decorated with @parametrize, in a parametrized sequence this surfaces at build() time, where the message alone doesn't say which waveform produced it.
Nice catch! Well in the error stack, it should be possible to trace back which classes emitted the error, and hence deduce which waveform produced it, but it should be possible to improve this as well :)

I let you push your changes, and I am looking for working with you on this PR! Good luck and best.

[jaewonyun1234]

Thanks for the review and merge! If it's useful, I'd like to take the register/ module next — base_register.py alone has 10 error messages that state a rule without reporting the input, plus a few more across register.py, register_layout.py and mappable_reg.py (~28 total).

Two questions since I'd like to keep a decent pace: would you prefer PRs grouped by module like that, or smaller file-by-file ones? And how many open PRs from me at a time is comfortable for you?

[a-corni]

Taking over the register/module would definitely be useful! PRs grouped by module is great, no need to make file-by-file changes. You can cover the entire package if you want and open as many MRs you can (with one MR per module, I am fine with this).