fix: Allow image coordinate irregularity equal to the tolerance#6915
Conversation
The expected failure is because of erroneous warning for regular coordinates with coarse granularity within the spacing.
The change prevents an erroneous warning for regular date-time
coordinates whose minimum step has coarse granularity in the internal
date-time representation. For such a step, the right-hand side can
evaluate to a time delta of zero (depending on how the object rounds
when multiplied), and the former `<` inequality cannot be satisfied.
Also, the change conforms the function to its docstring ("at most") and
to the documentation of `Config.image_rtol` ("maximal allowable sampling
difference").
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6915 +/- ##
==========================================
+ Coverage 89.29% 89.32% +0.02%
==========================================
Files 344 344
Lines 74554 74577 +23
==========================================
+ Hits 66573 66613 +40
+ Misses 7981 7964 -17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hoxbro
left a comment
hoxbro
left a comment
There was a problem hiding this comment.
Just want to verify with you that the tests fails without the suggested change.
Also have you been used AI for these changes?
| """ | ||
| Constructing an Image with a date-time coordinate warns about irregular sampling | ||
| if, approximately speaking, one less than the ratio of the max and min steps | ||
| exceeds the default `image_rtol`. | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Constructing an Image with a date-time coordinate warns about irregular sampling | |
| if, approximately speaking, one less than the ratio of the max and min steps | |
| exceeds the default `image_rtol`. | |
| """ |
Docstrings is not needed for tests.
There was a problem hiding this comment.
I understand. I see the docstring as a natural place to explain the intent of a non-obvious test. Here, in particular, it relates the test to image_rtol, a relationship that isn't apparent from the test's parameters, which are declared independently of image_rtol. Would a comment be preferred over a docstring?
| """ | ||
| Constructing an Image with an evenly spaced date-time coordinate does not warn | ||
| about irregular sampling, even when the ratio of the date-time granularity to | ||
| the coordinate step is greater than the default `image_rtol` (or twice | ||
| `image_rtol` for `datetime.datetime` types because of how they round when | ||
| multiplied by a `float`). | ||
| """ |
There was a problem hiding this comment.
| """ | |
| Constructing an Image with an evenly spaced date-time coordinate does not warn | |
| about irregular sampling, even when the ratio of the date-time granularity to | |
| the coordinate step is greater than the default `image_rtol` (or twice | |
| `image_rtol` for `datetime.datetime` types because of how they round when | |
| multiplied by a `float`). | |
| """ |
| # |granularity / step| = 1/400 = 0.0025 | ||
| [dt.datetime(2017, 1, 1, 0, 0, 0, ms) for ms in [0, 400]], | ||
| # |granularity / step| = 1/800 = 0.00125 |
There was a problem hiding this comment.
Why did you choose two different ratios?
There was a problem hiding this comment.
Because datetime.timedelta and np.timedelta64 instances round differently when multiplied by a float such as image_rtol, the range of coordinate steps that will cause the false warning differs for the two types. (A datetime.timedelta instance rounds to the nearest microseconds, whereas a np.timedelta64 truncates toward zero.) Specifically, with image_rtol = 0.001, the false warning occurs for datetime.datetime steps of less than 500 microseconds and for np.datetime64 steps of less than 1000 units. For each type, I chose a step that is a little smaller than the largest sufficient step for that type.
One of the added tests, namely
I used no AI. |
2 participants
Description
The change prevents an erroneous warning such as the following for regular date-time coordinates whose minimum step has coarse granularity in the internal date-time representation:
For such a step, the right-hand side of the inequality in
holoviews.core.util.validate_regular_samplingcan evaluate to a time delta of zero (depending on how the time-delta object rounds when multiplied), and the former<inequality cannot be satisfied. Also, the change conforms the function to its docstring ("at most") and to the documentation ofConfig.image_rtol("maximal allowable sampling difference").Fixes #6914.
Checklist