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

Improving readability/consistency of errors/exceptions in gempyor (second batch) #387

Merged
merged 59 commits into from
Dec 4, 2024

Conversation

emprzy
Copy link
Collaborator

@emprzy emprzy commented Nov 6, 2024

Describe your changes.

This pull request adjusts syntax, clarity, and formatting of errors/exceptions in gempyor's model_info.py, outcomes.py, seeding.py, seir.py, and simulation_component.py. This is the second of many sequential PR's to address the lack of consistency in formatting and style in gempyor errors/exceptions.

ADDITION SINCE TENDING TO SUGGESTIONS: This pull request also adjusts the regex verbiage in unit test files to match on specific values, rather than just the wording of the error messages that they are testing.

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 a logger) for stray warnings or errors. See GH- #310.

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.

emprzy and others added 2 commits November 1, 2024 11:26
Includes model_info.py, outcomes.py, seeding.py, seir.py, and simulation_component.py (this last one didn't require any edits).
@emprzy
Copy link
Collaborator Author

emprzy commented Nov 6, 2024

Creating this PR so that I can easily see the conflicts that need to be resolved. This is not ready for review yet!

@pearsonca
Copy link
Contributor

Creating this PR so that I can easily see the conflicts that need to be resolved. This is not ready for review yet!

Fine for this go round, but consider using the "draft PR" functionality in the future.

@emprzy
Copy link
Collaborator Author

emprzy commented Nov 8, 2024

Creating this PR so that I can easily see the conflicts that need to be resolved. This is not ready for review yet!

Fine for this go round, but consider using the "draft PR" functionality in the future.

Ah, had no idea this functionality existed. Great, thank you!!

@TimothyWillard
Copy link
Contributor

Can we modify this PR to merge into the dev branch instead of main? I don't see any breaking changes for main at a glance, but I would like to move us towards merging feature branches into the dev branch. I think this will be a great addition to the December release!

@emprzy emprzy changed the base branch from main to dev November 8, 2024 20:15
Copy link
Contributor

@TimothyWillard TimothyWillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make all of the quick things suggestions to make it a breeze to get through everything this time. Like before, mostly nit-picky things relating to shortening messages and removing awkward punctuation, so hopefully quick to get through. One thing that is not in the code is this warning that came from the CI:

=============================== warnings summary ===============================
tests/parameters/test_parameters_class.py:286
tests/parameters/test_parameters_class.py:286
  /home/runner/work/flepiMoP/flepiMoP/flepimop/gempyor_pkg/tests/parameters/test_parameters_class.py:286: DeprecationWarning: invalid escape sequence '\:'
    f"Provided file dates span '{timeseries_start_date}( 00\:00\:00)?' to '{timeseries_end_date}( 00\:00\:00)?', "

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================= 410 passed, 2 warnings in 217.92s (0:03:37) ==================

This may have snuck in with GH-313, but can we fix here? See: https://github.com/HopkinsIDD/flepiMoP/actions/runs/11748501992/job/32732711221.

flepimop/gempyor_pkg/src/gempyor/model_info.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/model_info.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/model_info.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/model_info.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/model_info.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/seir.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/seir.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/seir.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/tests/seir/test_seir.py Outdated Show resolved Hide resolved
flepimop/gempyor_pkg/src/gempyor/model_info.py Outdated Show resolved Hide resolved
the function `test_raises_not_implemented_error()` in test file `test_read_df.py` on account that it was receiving a `ValueError` when it expected a `NotImplementedError`. I switched the error within `utils.py:read_df()` to be an expected `NotImplementedError`.
Copy link
Contributor

@TimothyWillard TimothyWillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move forward with this in this release. I know there are more rounds of error improvements coming.

@emprzy emprzy merged commit 0aca24f into HopkinsIDD:dev Dec 4, 2024
2 checks passed
@emprzy
Copy link
Collaborator Author

emprzy commented Dec 4, 2024

Let's move forward with this in this release. I know there are more rounds of error improvements coming.

This PR marks the completion of this degree of error improvements in the files in the flepiMoP/flepimop/gempyor_pkg/src/gempyor folder. At a high level, do you see the next wave of improvements being...

  1. more breadth of improvements (pick a different folder in flepiMoP and improve the errors in those files the same way I have done these)
    or
  2. more depth of improvements within the folder gempyor folder I've already been working in. Not super positive what this one would entail but I'm sure there's more improvements we could make (e.g., a logger maybe? Idrk)

@TimothyWillard
Copy link
Contributor

This PR marks the completion of this degree of error improvements in the files in the flepiMoP/flepimop/gempyor_pkg/src/gempyor folder. At a high level, do you see the next wave of improvements being...

This is just a misunderstanding on my part then. For whatever reason I thought there was another round coming. I think we can close GH-281 then and any further work on error messages would be hyper targeted under the umbrella of other issues (i.e. GH-276).

@emprzy
Copy link
Collaborator Author

emprzy commented Dec 4, 2024

This PR marks the completion of this degree of error improvements in the files in the flepiMoP/flepimop/gempyor_pkg/src/gempyor folder. At a high level, do you see the next wave of improvements being...

This is just a misunderstanding on my part then. For whatever reason I thought there was another round coming. I think we can close GH-281 then and any further work on error messages would be hyper targeted under the umbrella of other issues (i.e. GH-276).

Mmmm I see. Okay let's close GH #281

@emprzy emprzy deleted the gempyor_error_improvements_part2 branch December 6, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants