Refactor: Consolidate Inlined Throw Strings into throws.jl #575
Conversation
…throws.jl for consistency
…throws.jl for consistency
…throws.jl for consistency
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #575 +/- ##
==========================================
- Coverage 84.11% 82.97% -1.15%
==========================================
Files 102 63 -39
Lines 6253 5299 -954
==========================================
- Hits 5260 4397 -863
+ Misses 993 902 -91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thanks for looking into this, @niranjannagumalli , much appreciated!
There are a few things that would be important to do to have this be more manageable.
-
A bunch of the error messages had some context-dependent information in them like sizes or values of parameters. These should not be lost. One way to do it can be to create
SOME_ERROR_MSG(a,b,c) = "... $a ..."instead ofconst SOME_ERROR_MSG = "..." -
Error messages that are used only ones should be kept as string literals -- no point in complicating the code and moving them away
-
Having one single master file of error messages seems a bit unwieldly, especially if an error message is otherwise used in a single file. Just defining the constant at the top of the file should be enough. For error messages used in multiple files it can make sense to use the aforementioned master file, but otherwise let's avoid that.
|
To corroborate further on Stefan's remarks, I would reckon that it would prove wiser to adhere to a few generic error messages and only perhaps offer further clarification if they arise from some obscure consideration. For example,
If the documentation includes the necessary information, then it may also be wise to add:
|
|
I will push against parts of that last suggestion. It depends on who the consumer of the error message is. If the consumer is someone fairly knowledgeable of the internals of the package, they can be generic. When I get an error message in this package, I care more about the stacktrace than the message itself, and do not necessarily read it. But the greatest value of the error messages is for the newbies. The perfect error message is the one that teaches the new user how to do something in a better way. Error messages should be very detailed, specific, actionable, and suggesting of possible sources of errors. They should be nicely formatted and contain context-specific data like sizes and nicely formatted types. In julia it is also possible to split the message into the proper error message and a "hint", but that is not very applicable to this discussion. Error messages also can have custom The rust compiler is a good example of a tool with state of the art error messages. The SciML ecosystem also has invested a lot of time into making their error messages more helpful to new users. Bringing the discussion back to this specific PR: plenty of the really valuable and interesting work in making the messages more useful to the user is out of scope to this PR. Nonetheless the reorganization started here is very valuable in identifying places where error messages should be standardized and put into const strings for reuse (or more probably, into functions that generate such strings containing a lot of additional context information). |
If an explanation is no longer than a paragraph, it should probably be part of the error message, for the sake of making onboarding non-technical users more easily. |
|
@Krastanov Thanks for the suggestions. I've made the requested changes. Could you please have a look and let me know if everything looks fine?
|
There was a problem hiding this comment.
Thanks for continuing on this. It is a type of issue that is a slog to finish, especially as there is a lot of nitpicky stuff about how to set it up when it is touching more than 30 files. FYI, it is a good idea to do this type of work in multiple small PRs, touching only a few files at a time -- that way you avoid merge conflicts and you can discuss the nitty-gritty details and iterate without having to redo a ton of work each time.
There seems to be a bunch of merge conflicts. I will do a quick superficial review, but the merge conflicts would need to be addressed before we can do something more throughout.
There was a problem hiding this comment.
Any reason for changes to this file? Avoid unrelated changes and undo this one if it is not needed.
| const THROW_INVALID_CUDA_ARG = | ||
| "First argument to @run_cuda should be a function call" | ||
| macro run_cuda(call, ndrange) | ||
| # destructure the kernel call | ||
| Meta.isexpr(call, :call) || throw(ArgumentError("first argument to @run_cuda should be a function call")) | ||
| Meta.isexpr(call, :call) || throw(ArgumentError(THROW_INVALID_CUDA_ARG)) |
There was a problem hiding this comment.
This string seems to be used in a single location. I think it makes more sense to just keep it as a literal, the way it was already set. Moving it makes things less easy to follow without significant benefit (unlike the situation where the same string is used all the time). This comment probably applies to a lot of other places.
| include("../../src/throws.jl") | ||
| using .Throws |
There was a problem hiding this comment.
this does not seem to be used
| "The group algebra must be commutative when using a single `repr` function,\ | ||
| which is not the case here. Please specify separate `A_repr` and `B_repr` \ | ||
| instead of a single `repr`. The default choice of\ | ||
| `A_repr=right_repr_matrix, B_repr=left_repr_matrix` is frequently sufficient." |
There was a problem hiding this comment.
you need a whitespace a the start of lines or before the \, otherwise the words get linked together.
| throw(ErrorException("Model exceeded time limit")) | ||
| else | ||
| throw(ErrorException("Model failed to solve: $(termination_status(model))")) | ||
| throw(ErrorException("Model failed to solve :$(termination_status(model))")) |
There was a problem hiding this comment.
this seems wrong or maybe I am misunderstanding the change?
| const THROW_NQUBITS = | ||
| "Unable to perform the requested operation due to encountering a mismatch \ | ||
| between the number of qubits in the provided arguments." |
| n == nqubits(p[1]) == nqubits(p[2]) || throw(ArgumentError(lazy""" | ||
| You are attempting to construct a `PauliChannel` but have provided Pauli operators | ||
| that are not all of the same size (same number of qubits). | ||
| Please ensure that all of the Pauli operators being provided of of the same size. | ||
| """)) | ||
| n == nqubits(p[1]) == nqubits(p[2]) || throw(ArgumentError(""" | ||
| You are attempting to construct a `PauliChannel` but have provided Pauli operators | ||
| that are not all of the same size (same number of qubits). | ||
| Please ensure that all of the Pauli operators being provided of of the same size. | ||
| """)) |
There was a problem hiding this comment.
removing the new padding in front is not needed. Tripple-quote strings remove that automatically:
| rank(md)==n || throw(ArgumentError(lazy"""Attempting to convert a `Stabilizer`-like object to `GeneralizedStabilizer` object failed, because the initial state does not represent a pure state. Currently only pure states can be used to initialize a `GeneralizedStabilizer` mixture of stabilizer states.""")) | ||
| rank(md)==n || throw(ArgumentError("""Attempting to convert a `Stabilizer`-like object to `GeneralizedStabilizer` object failed, | ||
| because the initial state does not represent a pure state. | ||
| Currently only pure states can be used to initialize a `GeneralizedStabilizer` mixture of stabilizer states.""")) |
There was a problem hiding this comment.
Long lines are not a problem, I have nothing against them. If you want to make the long lines into a paragraph, make sure to left-pad them so that they are neatly aligned with where they start and use \ so that they do not print with forced newlines (the printer can figure out its own wrapping, appropriate for the size of the terminal of the user).
Example of the features of """ strings:
| sMY(q, args...) = if q<=0 throw(NoZeroQubit) else new(q,args...) end | ||
| sMY(q, args...) = if q<=0 throw(ArgumentError(THROW_NO_ZERO_QUBIT)) else new(q,args...) end |
There was a problem hiding this comment.
NoZeroQubit or NO_ZERO_QUBIT seems more readable to me than ArgumentError(THROW_NO_ZERO_QUBIT). Generally, if you know what error type you will be using, you can directly store that error type in your constant, instead of storing just the string.
| const THROW_INVALID_ACTION_QUBITS = | ||
| "The tableau and the provided operator need to act on the same number of qubits. | ||
| Consider specifying an array of indices as a third argument to the `apply!` function to avoid this error." | ||
|
|
||
| const THROW_NO_ZERO_QUBIT = | ||
| "Qubit indices have to be larger than zero, | ||
| but you are attempting to create a gate acting on a qubit with a non-positive index. Ensure indexing always starts from 1." | ||
|
|
||
| THROW_MISSING_PACKAGE(func, pack) = | ||
| "You've invoked `$func` which depends on the package `$pack` but you have not installed or imported it yet. | ||
| Immediately after you import `$pack`, the $func will be available." | ||
|
|
||
| THROW_CHECKS_MISSING(code) = | ||
| "Codes of the type $code do not have separate X and Z parity checks, either because they are not a CSS code | ||
| and thus inherently do not have separate checks, or because its separate checks are not yet implemented in this library." |
There was a problem hiding this comment.
These probably should not have forced newlines.
closes issue #532