Fix get_integer_time precision and zero handling by Saptami191 · Pull Request #494 · mllam/neural-lam

Saptami191 · 2026-03-22T18:35:49Z

Describe your changes

Fix get_integer_time precision issues and zero timedelta handling.

Uses integer microseconds instead of float computation to avoid rounding errors and ensure correct unit detection.

No additional dependencies.

Issue Link

Closes #468

Type of change

🐛 Bug fix (non-breaking change that fixes an issue)
✨ New feature (non-breaking change that adds functionality)
💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
📖 Documentation (Addition or improvements to documentation)

Checklist before requesting a review

My branch is up-to-date with the target branch - if not update your fork with the changes from the target branch (use pull with --rebase option if possible).
I have performed a self-review of my code
For any new/modified functions/classes I have added docstrings that clearly describe its purpose, expected inputs and returned values
I have placed in-line comments to clarify the intent of any hard-to-understand passages of my code
I have updated the README to cover introduced code changes
I have added tests that prove my fix is effective or that my feature works
I have given the PR a name that clearly describes the change, written in imperative form
I have requested a reviewer and an assignee

Checklist for reviewers

Each PR comes with its own improvements and flaws. The reviewer should check the following:

the code is readable
the code is well tested
the code is documented (including return types and parameters)
the code is easy to maintain

Author checklist after completed review

I have added a line to the CHANGELOG describing this change, in a section
reflecting type of change:
- fixes

Checklist for assignee

PR is up to date with the base branch
the tests pass
(if the PR is not just maintenance/bugfix) the PR is assigned to the next milestone. If it is not, propose it for a future milestone.
author has added an entry to the changelog (and designated the change as added, changed, fixed or maintenance)
Once the PR is ready to be merged, squash commits and merge the PR.

Saptami191 · 2026-03-22T18:57:36Z

@joeloskarsson Recreated this PR after fixing earlier issues with the template and workflow. The code, tests, and linting are now clean and aligned with project guidelines. Would appreciate a review.

kshirajahere

I think the integer-microseconds direction is the right fix, but I think there is still few thing worth addressing before merge.

kshirajahere · 2026-03-22T19:51:13Z

+        (0, 'seconds')
    """
-    total_seconds = tdelta.total_seconds()
+    total_microseconds = (


The new implementation is fine, but this helper’s contract is still inconsistent across the tree: compute_standardization_stats.py still calls get_integer_time(step_length.total_seconds()), i.e. with a float rather than a timedelta. I think we should either update that caller in the same slice or explicitly tighten/document the contract here.

kshirajahere · 2026-03-22T19:51:40Z

+    assert get_integer_time(timedelta(0)) == (0, "seconds")
+
+
+def test_milliseconds():


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 (timedelta(days=0.001) -> (86400, "milliseconds")) here as well, rather than leaving that behavior covered only by the doctest.

This is actually already tested in the docstring.

kshirajahere · 2026-03-22T19:56:31Z


 - Fix typo in `ar_model.py` that causes `AttributeError` during evaluation [\#204](https://github.com/mllam/neural-lam/pull/204) @ritinikhil

+- Fix `get_integer_time` to avoid floating-point precision issues and correctly handle zero timedelta [#469](https://github.com/mllam/neural-lam/pull/469) @Saptami191


Shud point to #494 instead of #469

Thanks for the review! I hve addressed all three points.

observingClouds

Thanks for this PR. I left a few comments and suggest a few changes.

observingClouds · 2026-03-23T08:57:26Z

+    assert get_integer_time(timedelta(0)) == (0, "seconds")
+
+
+def test_milliseconds():


This is actually already tested in the docstring.

observingClouds · 2026-03-23T08:58:26Z

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.

@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.
Should I also add a doctest for negative timedelta

get_integer_time(timedelta(days=-7))
(-1, 'weeks')

it will represent a duration in the past 7 days ago.
or is that out of scope for this PR?

Saptami191 · 2026-04-13T20:44:11Z

@observingClouds Should get_integer_time explicitly support negative timedeltas (e.g., timedelta(days=-7) -> (-1, "weeks")), or is that out of scope for this PR?

observingClouds · 2026-04-14T05:12:31Z

+def test_negative():
+    assert get_integer_time(timedelta(days=-7)) == (-1, "weeks")


Yes, please add this test to the doctest and then all is covered by the doctest and we can remove all tests here

Saptami191 · 2026-04-14T09:38:28Z

@observingClouds Hii! Requested changes are done and all tests have been passed

@Saptami191

Re-applies @Saptami191's PR mllam#494 onto current main with these adjustments: - Dropped the compute_standardization_stats.py edit: the second caller it was fixing (`get_original_window_indices`) no longer exists in current main, and the surviving caller already passes a timedelta (not a float) - Added thousands separators (1_000_000 etc.) to the microsecond constants for readability - Added a one-line comment explaining why we avoid total_seconds() floats - Placed CHANGELOG entry in the current [unreleased] section under Fixed Co-Authored-By: Shoko <Saptami191@users.noreply.github.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

sadamov

@observingClouds @Saptami191 I just rebased this PR onto main. Feel free to have a look and merge if you agree.

kshirajahere reviewed Mar 22, 2026

View reviewed changes

sadamov self-requested a review March 23, 2026 05:10

observingClouds requested changes Mar 23, 2026

View reviewed changes

observingClouds self-assigned this Mar 23, 2026

sadamov requested review from kshirajahere and observingClouds and removed request for kshirajahere and sadamov April 1, 2026 08:32

sadamov added the bug Something isn't working label Apr 13, 2026

observingClouds requested changes Apr 14, 2026

View reviewed changes

sadamov force-pushed the fix-get-integer-time-v2 branch from 70af30f to 9e2d2cb Compare June 5, 2026 07:13

Merge branch 'main' into fix-get-integer-time-v2

8b4f0c5

sadamov requested a review from observingClouds June 5, 2026 07:35

sadamov approved these changes Jun 5, 2026

View reviewed changes

		assert get_integer_time(timedelta(0)) == (0, "seconds")


		def test_milliseconds():


		- Fix typo in `ar_model.py` that causes `AttributeError` during evaluation [\#204](https://github.com/mllam/neural-lam/pull/204) @ritinikhil

		- Fix `get_integer_time` to avoid floating-point precision issues and correctly handle zero timedelta [#469](https://github.com/mllam/neural-lam/pull/469) @Saptami191

		def test_negative():
		assert get_integer_time(timedelta(days=-7)) == (-1, "weeks")

Conversation

Saptami191 commented Mar 22, 2026

Describe your changes

Issue Link

Type of change

Checklist before requesting a review

Checklist for reviewers

Author checklist after completed review

Checklist for assignee

Uh oh!

Saptami191 commented Mar 22, 2026

Uh oh!

kshirajahere left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

observingClouds left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saptami191 Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saptami191 commented Apr 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Saptami191 commented Apr 14, 2026

Uh oh!

sadamov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Saptami191 Mar 23, 2026 •

edited

Loading