Repository navigation
fix: align naive datetimes independent of the local UTC offset - #1146
Open
charanteja25 wants to merge 1 commit into
Open
charanteja25 wants to merge 1 commit into
charanteja25 wants to merge 1 commit into
Conversation
align_datetime used datetime.timestamp() for naive datetimes. That call applies the local UTC offset of the machine, so alignment to an interval was wrong when the offset is not a multiple of the interval (for example UTC+05:30 and a one-hour interval). Naive datetimes are now aligned with timedelta arithmetic relative to a naive epoch. Aware datetimes are unchanged. Add a regression test that runs with the process timezone set to UTC+05:30. Signed-off-by: charanteja25 <charanteja.teja25@gmail.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Is your feature request related to a problem?
Yes, a bug.
align_datetime(openstef_core.utils.datetime) aligns a naive datetime withdatetime.timestamp(). For a naive datetime, this call applies the local UTC offset of the machine. If that offset is not a multiple of the interval, the result is wrong. Example: UTC+05:30 (India) and a one-hour interval.Reproduce:
Expected
2020-01-01 10:00:00, actual2020-01-01 10:30:00. WithTZ=UTCthe result is correct.Impact:
TZ=UTC: 4 intest_datetime.py(test_align_datetime_naive) andtest_run_returns_evaluation_reportinopenstef-beam.align_datetimeis used by the evaluation window iterator (window_iterators.py). With naive datetimes, window boundaries can shift by 30 minutes in such time zones, and the error is silent.Describe the solution you'd like
For naive datetimes, align with
timedeltaarithmetic relative to a naive epoch (datetime(1970, 1, 1)). Aware datetimes stay unchanged. A new regression test runs with the process timezone set to UTC+05:30 and fails without the fix.Describe alternatives you've considered
Affected package(s)
Would this require breaking changes to the public API?
No. Only naive datetimes at non-whole-hour offsets give a different (correct) result.
Additional context
Related to #715 (timezone robustness). This is a concrete case of a silent problem with naive datetimes.
Question for the review: for aware datetimes, the function aligns on UTC epoch boundaries, not on local wall-clock boundaries. For example, 10:45 in a +05:30 zone with a one-hour interval floors to 09:30 local time, not 10:00. I did not change this, because it may be intended. Is it?
Checks:
poe all --checkpasses on my machine in the +5:30 time zone, withoutTZ=UTC: 1131 tests and 110 doctests passed. The 5 failures above are gone.