Improving readability/consistency of errors/exceptions in gempyor.
#313
Conversation
…les. Adjusted formatting, punctuation, clarity, syntax, etc. in the errors/exceptions in `gempyor`'s initial_conditions.py, parameters.py, compartments.py, config_validator.py, inference.py. This will likely be one of three pulls to address HopkinsIDDGH-281.
There was a problem hiding this comment.
I didn't comment on all of the exceptions, but I'm sure it's clear what my main issues are at this point. Overall, I want to emphasize:
- Making exceptions 1-2 short sentences with no new lines/awkward spacing or punctuation,
- Avoiding redundancy like saying the word "error" in an exception, and
- Avoiding unnecessary use of f-strings (should use the right tool for the job).
I think it's also worth thinking about what exception message formatting norms we want to follow. For example, I usually try to do backticks for variable names and single quotes around values so:
raise RuntimeError(f"The value of `x` is '{x}', but must be between 1 and 10.")Not saying we have to follow that convention, but we need a convention. Finally, I pointed out one or two cases of this but we should make sure that we are using the right type of exception for each of these. While I think most of these are actually ValueErrors it's worth double checking to be sure. Link to docs for reference: https://docs.python.org/3/library/exceptions.html.
| f"The provided dictionary does not allow to isolate a compartment: {comp_dict} " | ||
| f" isolate {self.compartments[mask]} from options {self.compartments}. " | ||
| f"The get_comp_idx function was called by '{error_info}'." |
There was a problem hiding this comment.
Looks like there's a double space between the first and second line. I'm also not super familiar with the context here, but can this be edited for grammar as well. Something like: "The provided dictionary does not allow an isolated compartment: {comp_dict}. It is isolated from the options {self.compartments}. Additional info: {error_info}."?
There was a problem hiding this comment.
I happen to be looking at this particular exception again for reasons unrelated to this PR and I now think this should be a LookupError: https://docs.python.org/3/library/exceptions.html#LookupError. I think that better describes the issue at hand here.
Co-authored-by: Timothy Willard <[email protected]>
Making '' and `` conventions consistent, removing unnecessary f-strings, added some warnings.warn() statements instead of print() statements.
…y/flepiMoP into gempyor_error_improvements
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
Co-authored-by: Timothy Willard <[email protected]>
| @@ -134,14 +134,16 @@ def __init__( | |||
| print("loaded dates:", df.index) | |||
| raise ValueError( | |||
| f"Issue loading file '{fn_name}' for parameter '{pn}': " | |||
| f"The 'date' entries of the provided file do not include all the days specified to be modeled by the config." | |||
| f"Provided file dates span '{str(df.index[0])}' to '{str(df.index[-1])}', " | |||
| f"but the config dates span '{ti}' to '{tf}'." | |||
There was a problem hiding this comment.
I think precises error messages help a lot, i know there is a discussion in keeping them concise and all, but if we look at e.g https://iddynamicsjhu.slack.com/archives/C07PBSVRJ05/p1729087436855849, it is clear that in a context where the person generating the config is often different from the person running flepiMoP, the more informative the message is the faster is the communication
There was a problem hiding this comment.
e.g in the linked error, the dates are the same but some dates are duplicated. If we want to keep short error message, we could differenciate the two cases:
- the end-start dates are different
- the start-end dates are the same but number of elements is not the same.
There was a problem hiding this comment.
I can't follow the slack link to see what happened there, but perhaps fixing these exceptions to catch the right thing this is more of an issue for GH-277 then? Can just put the number of days back in the meantime. There is already a comment #276 (comment) discussing this set of errors.
There was a problem hiding this comment.
agree on keeping it that way here, and the need for specific errors later. In your linked you have identified what has been needed in this slack message (number of date vs just end and start date). adding a screenshot in case:
There was a problem hiding this comment.
Heard. Returning the error message to the original level of verbosity!
| @@ -134,14 +134,16 @@ def __init__( | |||
| print("loaded dates:", df.index) | |||
| raise ValueError( | |||
| f"Issue loading file '{fn_name}' for parameter '{pn}': " | |||
| f"The 'date' entries of the provided file do not include all the days specified to be modeled by the config." | |||
| f"Provided file dates span '{str(df.index[0])}' to '{str(df.index[-1])}', " | |||
| f"but the config dates span '{ti}' to '{tf}'." | |||
There was a problem hiding this comment.
e.g in the linked error, the dates are the same but some dates are duplicated. If we want to keep short error message, we could differenciate the two cases:
- the end-start dates are different
- the start-end dates are the same but number of elements is not the same.
| print(f"dimension: {dimension}") | ||
| raise e | ||
| raise Exception( | ||
| f"Error {e}: " |
There was a problem hiding this comment.
n.b., there are some other case to apply this style to.
| rf"^Issue loading file '{tmp_file}' for parameter 'sigma': " | ||
| rf"the number of non-'date' columns is '{actual_columns}', expected '{mock_inputs.number_of_subpops()}' " | ||
| rf"\(number of subpopulations\) or one\.$" |
There was a problem hiding this comment.
This is a very prescriptive error message match - which means any future tweaks to that error message have to be also maintained with changes here.
Is there something that we want to continue to ensure appears in the error message (e.g. something to do with variable introspection) without specifying the exact error message?
| f"Issue loading file '{tmp_file}' for parameter 'sigma': " | ||
| f"Provided file dates span '{timeseries_start_date}( 00\:00\:00)?' to '{timeseries_end_date}( 00\:00\:00)?', " | ||
| f"but the config dates span '{mock_inputs.ti}' to '{mock_inputs.tf}'.$" |
There was a problem hiding this comment.
similar comment to previous: too prescriptive on exact match to error message.
| @@ -44,7 +44,7 @@ def test_IC_allow_missing_node_compartments_success(self): | |||
| sic.get_from_config(sim_id=100, modinf=s) | |||
|
|
|||
| def test_IC_IC_notImplemented_fail(self): | |||
| with pytest.raises(NotImplementedError, match=r".*unknown.*initial.*conditions.*"): | |||
| with pytest.raises(NotImplementedError, match=r"^Unknown initial conditions method \[received: .*?\]\.$"): | |||
There was a problem hiding this comment.
This is both too prescriptive and not actually checking enough - because the received part is an open ended regex, doesn't matter what's there. We should be checking that the error message when offered XYZ for method replies that XYZ is garbage - that is, we should be checking for XYZ in the message.
….py, and one regex in test_ic.py
Updating an error message to be more streamlined, and also match the RegEx in test_parameters_class.py
Allowing black to reformat my code
There was a problem hiding this comment.
I think this is fine.
I am still annoyed about the test condition in test_ic, but I think that's better resolved by a PR targeting better testing + more informative error conditions.
My philosophy on error messages is that they need to inform the user about what they did wrong. So the tests for error message should ensure that the message matches the name of the bad argument + what the user provided.
Noted. Will have more questions in the future on which specific test cases are optimal vs. sub-optimal, but I'm excited to get this merged :) |
Describe your changes.
This pull request adjusts syntax, clarity, and formatting of errors/exceptions in
gempyor'sinitial_conditions.py,parameters.py,compartments.py,config_validator.py, andinference.py. This will be one of many sequential PR's to address the lack of consistency in formatting and style ingempyorerrors/exceptions.This pull request does NOT attempt to narrow or refine which type of errors are thrown, nor does it address the poor practice of using
print()statements (as opposed to alogger) for stray warnings or errors. See GH- #310 for that.What does your pull request address? Tag relevant issues.
This pull request addresses GH- #281.
By splitting this up into multiple PR's, hopefully it will make it easier to review, and also make it easier for me to be dynamic in the solutions I implement if anyone has any suggestions for the way I am doing this.