Skip to content
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

Enhanced ValueError messages #148

Closed
romainfd opened this issue May 11, 2021 · 6 comments
Closed

Enhanced ValueError messages #148

romainfd opened this issue May 11, 2021 · 6 comments
Assignees
Labels

Comments

@romainfd
Copy link

When using values out of range (examples below for qubit positions and pulse detuning), the ValueError exception could indicate the minimum/maximum authorized value to make fixing the issue faster. The value is used in the test just above so printing it looks easy at first glance.

  • Illustration for min qubits distance
    image

  • Illustration for max pulse detuning
    image

@HGSilveri
Copy link
Collaborator

Hi @romainfd , thanks for the suggestion! I will tag it as a "good first issue" for whoever wants to tackle it.

@HGSilveri HGSilveri added the good first issue Good for newcomers label May 11, 2021
@LaurentAjdnik
Copy link
Collaborator

Hi! I've been doing so for the new functions Register.hexagon() and Register.max_connectivity() (soon to be PR-ed), so I might just as well continue for others.

Fortunately, some of them already do, like Channel.validate_duration() for instance.

Anyhow, this will be a good opportunity to:

  • Make sure allowed values are always mentioned as part of Error messages
  • Harmonize the way they are included in strings (throughout the code, you'll find F-strings, .format() and perhaps others)

By the way, I found almost no checks for None on parameters. Testing against allowed values will result in a TypeError (see Ordering comparisons).

Do we want to add checks and throw an explicit ValueError with a message rather than this default behavior ? This is already the case in Register.draw(), SimulationResults.sample_final_state() and Variable.build() but nowhere else.

@HGSilveri
Copy link
Collaborator

@LaurentAjdnik thanks! I agree with your suggestions, although I think it would be better to do them in a separate PR if possible, otherwise the one you're currently working on might get too big.

Regarding checks for None on parameters, those do sound a bit of overkill to me, but I will accept them if you feel they are necessary.

HGSilveri pushed a commit that referenced this issue May 18, 2021
…), Enhanced `ValueError` messages (#148), Type hinting for `Register` (#16) (#155)

* Update register.py

* Add max_connectivy()

* Test

* Adding hexagon() and max_connectivity()

* Type hinting and parameters checks

* Shift atom generation in full layers for consistency

* Remove unused variables to avoid flake8 warnings

* Corrections for PR #155
@LaurentAjdnik
Copy link
Collaborator

@HGSilveri: You can assign this one to me, as it is related to #157.

HGSilveri pushed a commit that referenced this issue Jun 11, 2021
* Enhance ValueError messages

* Enhance ValueError messages in all files

* Correct typo in pulse.py
@HGSilveri
Copy link
Collaborator

@LaurentAjdnik If you're satisfied with the changes from your last PR, I'll close this, just let me know.

@LaurentAjdnik
Copy link
Collaborator

@LaurentAjdnik If you're satisfied with the changes from your last PR, I'll close this, just let me know.

Yup, let's close this one. We might PR a few adjustments in the future but no need to keep this issue open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants