-
Notifications
You must be signed in to change notification settings - Fork 256
Fix get_integer_time precision and zero handling #494
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All these newly added tests are already covered by the doctest. No need to replicate them here. If you think a case is missing in the docstring, please add them there.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @observingClouds thanks for the review! yeah all are tested in docstring. i will remove this test file .actually it was part of proving the fix is correct.
it will represent a duration in the past 7 days ago. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # Standard library | ||
| from datetime import timedelta | ||
|
|
||
| # First-party | ||
| from neural_lam.utils import get_integer_time | ||
|
|
||
|
|
||
| def test_days(): | ||
| assert get_integer_time(timedelta(days=14)) == (2, "weeks") | ||
|
|
||
|
|
||
| def test_hours(): | ||
| assert get_integer_time(timedelta(hours=5)) == (5, "hours") | ||
|
|
||
|
|
||
| def test_zero(): | ||
| assert get_integer_time(timedelta(0)) == (0, "seconds") | ||
|
|
||
|
|
||
| def test_milliseconds(): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this PR is specifically about float-precision avoidance, I think it would be good to add one direct regression test for the issue-body example (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is actually already tested in the docstring. |
||
| assert get_integer_time(timedelta(milliseconds=1000)) == (1, "seconds") | ||
|
|
||
|
|
||
| def test_negative(): | ||
| assert get_integer_time(timedelta(days=-7)) == (-1, "weeks") | ||
|
|
||
|
|
||
| def test_float_days(): | ||
| assert get_integer_time(timedelta(days=0.001)) == (86400, "milliseconds") | ||
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.
The new implementation is fine, but this helper’s contract is still inconsistent across the tree:
compute_standardization_stats.pystill callsget_integer_time(step_length.total_seconds()), i.e. with a float rather than atimedelta. I think we should either update that caller in the same slice or explicitly tighten/document the contract here.