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.
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
Adding
check_parameter_positivity()
function toseir.py
#428base: dev
Are you sure you want to change the base?
Adding
check_parameter_positivity()
function toseir.py
#428Changes from 12 commits
80f98cb
ba683da
a5360ad
f60b62d
98c719d
b6686f0
1b841c6
34fdc1e
e3a364f
207ddde
78ddbae
133ef9e
c95bb04
9207dbb
6cc9696
148495a
e191981
39cdf6c
7951d11
2685ee4
61dd8dc
2a99037
cfd05f9
44f3fc2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that this error message will be quite lengthy and I'm not a fan of multi-line error messages. Is there a way that we could condense this down to one line? Maybe just error on the first negative parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I agree the message is lengthy, and it is sub-optimal for it to be multiple lines. But, I'm curious what your thoughts are on the usefulness of having an error message that only returns the first negative parameter, even if the function output knows where all of negative parameters are. Is there not a lot of added value in telling the user all of the columns that have negative values, so they can more quickly address the issue? I'm happy to change it to only show the first negative parameter value, but since the function inherently finds the others, I thought it would be useful to include.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that spewing out all the errors is too much noise - multiple errors often arise from single mistakes that need correcting.
I recommend that we limit the detailed part of the error message to the earliest time, for any parameter or subpop that there is a problem. However, I also think its worthwhile to indicate the totality of the problem. Something like
(possibly some of those elements can be curtailed if there are not multple subpops, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just chiming in that I asked @emprzy for this verbose error message (first date negative for all parameter and subpop negative) as it helps to debug configs without retrying (which, e.g for a RSV config takes 6/7 minutes), sorry Emily. Totally understand that we want to keep it light but I think a good diagnosis (e.g. a graph or something) is useful here, though perhaps not inside the simulate command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the more clear the error message is the most useful it would be practically, though understand it's long to print out everything for each subpop for example. In particular I think it would be useful as much information about the specific parameters? Maybe a simplification/modification of this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saraloo's suggestion seems like a reasonable compromise to me.
Minor: but maybe
starting from date YYYY-MM-DD, in parameters: ...
instead, notably no newline. Newlines can be annoying to format in unit tests matching on exception, see this prior version ofParameters
unit tests as an example:flepiMoP/flepimop/gempyor_pkg/tests/parameters/test_parameters_class.py
Lines 283 to 297 in 2ec9695
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re unit test matching, I'd reiterate that its overkill to match exact messages - for example here, should only be matching first bad date string (irrespective of what's around it), subpop id (ibid), and offending parameters (ibid).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyWillard , does this mean you propose leaving out the subpop information and just including the parameter names that are negative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, no, I just was conveying an edit to a portion of @saraloo's suggestion. The full change with my edit would be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TimothyWillard @saraloo @jcblemai
The error message now reads as follows:
ValueError: There are negative parameter errors in subpops ['56000', '44000', '30000'], starting from date 2023-03-19 in parameters ['alpha*1*1*1', 'sigma_OMICRON*1*1*1', '3*gamma*1*1*1'].
Is this what you had in mind? Happy to change it, just wanted to confirm before pushing.