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

close #1486 #1501

Closed
wants to merge 3 commits into from
Closed

close #1486 #1501

wants to merge 3 commits into from

Conversation

karanjogi
Copy link

Related issues

[Bug] Error when the number of Fatal cases are not changing with SIR-D model #1486

What was changed

Edited SIRD, SIRF and SEWIRF models to have a condition in case Fatal cases didn't change for a phase.
Also added a test case for the above models to check that the models do not break after the changes.

@lisphilar lisphilar added this to the Release 3.0.1 milestone Aug 23, 2023
@lisphilar lisphilar added the bug Something isn't working label Aug 23, 2023
Copy link
Owner

@lisphilar lisphilar left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request!!
Please find my comments for discussion.

@@ -177,15 +177,15 @@ def dimensional_parameters(self) -> dict[str, float | int]:
try:
return {
"alpha1 [-]": round(self._theta, 3),
"1/alpha2 [day]": round(self._tau / 24 / 60 / self._kappa),
"1/alpha2 [day]": round(self._tau / 24 / 60 / self._kappa) if self._kappa != 0 else np.NaN,
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry for my missing in the discussion #1486, but the same error could be raised for the other parameters except for theta. If rho_i = 0 and sigma = 0 are acceptable, could you remove the ZeroDivisionError and add "if" statements to the parameters?

Just a matter of preference, but we could remove != 0 because if not 0: is always True.

@@ -163,7 +163,7 @@ def dimensional_parameters(self) -> dict[str, float | int]:
"""Calculate dimensional parameter values.
Raises:
ZeroDivisionError: either kappa or rho_i for i=1,2,3 or sigma value was over 0
ZeroDivisionError: rho_i for i=1,2,3 or sigma value was 0
Returns:
Copy link
Owner

Choose a reason for hiding this comment

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

Please add | None to the type hint and add "or None" to line 171 etc. for documentation.

@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 8 times, most recently from 3f2215b to db12452 Compare August 29, 2023 13:40
@lisphilar lisphilar self-requested a review September 1, 2023 11:04
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 2 times, most recently from 5615635 to 12224c4 Compare September 7, 2023 21:38
@lisphilar
Copy link
Owner

@karanjogi ,
I'm sorry for bothering you again, but please let me know if you need any further assistance regarding this pull request.

@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 5 times, most recently from 99af80b to a2179d6 Compare September 16, 2023 12:10
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 4 times, most recently from 13eff2d to c517bb1 Compare October 3, 2023 09:11
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 3 times, most recently from c4c23b4 to 48adb28 Compare October 16, 2023 22:45
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 2 times, most recently from 0502114 to 8ca92d9 Compare October 18, 2023 11:46
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 4 times, most recently from a10412a to c102012 Compare October 25, 2023 21:02
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 2 times, most recently from e5e9d23 to fc15801 Compare November 3, 2023 05:33
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 4 times, most recently from feb5a10 to 1490caa Compare November 15, 2023 21:14
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 3 times, most recently from d4829d8 to fe27725 Compare November 23, 2023 10:52
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 4 times, most recently from b031992 to dad8178 Compare November 29, 2023 22:38
@github-actions github-actions bot force-pushed the karanjogi/issue1486 branch 4 times, most recently from 254b379 to 24dacfe Compare December 11, 2023 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants