Update increment rounding functionality #53
Merged
Conversation
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
nekevss
force-pushed
the
update-rounding-approach
branch
from
0db56fc to
8961afe
Compare
| // NOTE(nekevss): Is it worth returning a `RangeError` below. | ||
| debug_assert!(nanoseconds.abs() <= MAX_TIME_DURATION); | ||
| Self(nanoseconds) | ||
| } | ||
|
|
||
| // NOTE: `days: f64` should be an integer. |
There was a problem hiding this comment.
Is this going to be changed in a follow up? I guess there's too many call sites right now to change in this PR
There was a problem hiding this comment.
More than likely, yes. I'm pretty sure I wanted to be able to verify in all cases that it would be days: u64 or some equivalent before making the change.
src/rounding.rs
Outdated
Comment on lines
106
to
104
| fn is_cardinal(&self) -> bool { | ||
| self.result_floor().rem_euclid(2) == 0 | ||
| } |
There was a problem hiding this comment.
Is this a correct name? Aren't all positive numbers cardinal rather than just even ones?
There was a problem hiding this comment.
Yeah, the name doesn't match real well. The name was a bit more specification leaning, but definitely ambiguous.
jasonwilliams
approved these changes
Jun 16, 2024
jedel1043
reviewed
Jun 16, 2024
jedel1043
reviewed
Jun 16, 2024
jedel1043
reviewed
Jun 16, 2024
jedel1043
reviewed
Jun 16, 2024
nekevss
force-pushed
the
update-rounding-approach
branch
from
03d0642 to
0f105f6
Compare
nekevss
force-pushed
the
update-rounding-approach
branch
from
b44fb0e to
be58fb1
Compare
jedel1043
approved these changes
Jun 20, 2024
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.
This is still technically a draft. Although due to Duration rounding updates, I'd almost be in favor of merging this depending on feedback, primarily due to potential scope creep.
Overview:
NormalizedTimeDurationneeds to be ai128and not af64for safety as well as the new duration rounding algoritms. This means we need to be able to round bothi128andf64for the library. So this branch currently updates the approach to rounding to allow handling ofi128andf64at the same time, which allowsNormalizedTimeDurationto be ani128.@jedel1043 This is the rounding functionality I had mentioned on the GC call. If you have any thoughts on the API, let me know. There's probably an argument for a
PositiveIncrementRounderthat whereTis au64instead of adding*_as_positivemethods on the current approach.Tests / Scope Creep
Currently, this does break a couple unit tests for duration rounding, but I'm fairly certain that those have to do with truncation or some other issue in the duration rounding algorithm not the increment rounding itself (added some sanity unit tests to rounding), so I might look into this further.This goes back to the scope creep I mentioned above. I'm trying not to make too much of an omnibus PR, and the duration rounding as a whole needs to be overhauled to the new algorithms. But the new algorithms need rounding and😄NormalizedTimeDurationupdated, so I'm not entirely sure whether it's worth fixing the current tests with the current algorithm when the fixes can be completed while updating to the new algorithm. Not entirely sure, so any thoughts would be welcomedActually found the bug. It was converting
NormalizedTimeDuration->fractional_days