Implement lossless From methods for Duration conversions

[Wollaston]

This works towards closing #337 by making the implementations of From<std::time::Duration> for Duration and From<Duration> for std::time::Duration lossless. Thank you!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.11%. Comparing base (93192da) to head (b5a8008).
Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   84.01%   84.11%   +0.10%     
==========================================
  Files          22       23       +1     
  Lines        3691     3677      -14     
==========================================
- Hits         3101     3093       -8     
+ Misses        590      584       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ChristopherRabotin]

I think a simpler way to implement this functionality is by using the to_unit function on the hf_duration.

I haven't tested the following, so consider this a draft.

let seconds = hf_duration.to_seconds().floor() as u64; // Somehow check that this won't fail
let subsec_nanos = (hf_duration - Unit::Second * seconds).to_unit(Unit::Nanoseconds).floor() as u32; // Also check that this won't fail, probably using try_from as you initially recommended

std::time::Duration(seconds, subsec_nanos)

[ChristopherRabotin]

There's a better way to handle the subsec_nanos than what I wrote above. After the subtraction, we know that the duration has zero centuries. So, we can safely ignore that part and operate directly on nanoseconds.

use crate::NANOSECONDS_PER_SECOND; // https://docs.rs/hifitime/latest/hifitime/constant.NANOSECONDS_PER_SECOND.html
let subsec_nanos: u64 = (hf_duration - Unit::Second * seconds).nanoseconds / NANOSECONDS_PER_SECOND;
// Finally convert this to u32 safely.

[ChristopherRabotin]

Thanks for your PR @Wollaston ! I've proposed a simpler way to have a lossless conversion to std::time::Duration: it's lossless because the operations on Duration are lossless, including the unit conversions. The first floor subtracts anything above one second from the duration, thereby ensuring that the to_unit(Unit::Nanosecond) will only be seconds.

Is there a way we could test this?

[Wollaston]

Thanks for the review @ChristopherRabotin! In the original issue #337, part of the concern was the conversion from ints to floats to ints, which is lossy with the float conversion. In your draft, hf_duration.to_seconds() does do a float conversion, so my inclination is to stay with the original PR that avoids floats altogether.

As to the updated subsec_nanos calculation, I was not able to get that to compile due to the following error: "cannot multiply timeunits::Unit by u64 the trait std::ops::Mul<u64> is not implemented for timeunits::Unit". Because of that, perhaps it would be simpler overall to keep with the original calculation as it avoids any compile issues, or needing to implement behavior elsewhere.

I agree it makes sense to use the crate::NANOSECONDS_PER_SECOND instead of the one calculated locally here.

As to tests, I can draft some unit tests in the module (it only has these two implementations) and/or add some doc tests to some updated documentation for these implementations. I am not sure if there is a pattern you prefer - just let me know on that. They could test that the updated implementations are in fact lossless as a start.

I can update the PR accordingly once those questions are settled, and then squash the commits. Many thanks!

[ChristopherRabotin]

You're right. Let's go ahead with your original implementation using the crate's NANOSECONDS_PER_SECOND constant. As for negative durations, where we'd want to return std::time::Duration::ZERO, there's now a const fn signum function which I recommend using here.

[Wollaston]

Sounds good! Just to clarify, we still want to return std::time::Duration::ZERO in the case of negative durations, correct? I might be overlooking how to use const fn signum for these implementations. The use of u128::try_from(nanos).unwrap_or(0) seems to catch the cases of negative durations without needing to use const fn signum, so maybe I am missing the best way to use that.

And for the tests, do you have a preference in how the tests are added, or just that they are? Thanks!

[ChristopherRabotin]

Yes, I think it makes sense to return Duration::ZERO when the hifitime duration is negative. The advantage of using the const fn is that, I believe, it's generally faster to evaluate than the unwrap_or, even though functionally it should be the same I agree.

By the way, no need to squash the commits, I tend to just merge all of the commits.

[ChristopherRabotin]

Sorry, this PR fell off my radar. I'll try to release version 4.0.1 this weekend for a minor bug fix in the Python type hints. I was wondering whether I could also merge your work and release it then.

Thanks

Edit: I've found a work-around to my documentation issue that doesn't require a new hifitime version, so take your time.

[Wollaston]

Good morning Chris - I also got pulled away for a few things, but I am planning on writing tests today. That should be the last step for this PR, barring any feedback. The current branch includes the signum function as a first check/short-circuit, before proceeding with the rest of the logic. It's linked in this thread. If you had something else in mind, just let me know.

As to tests, I am adding some big number tests to show this update is lossless. I saw that you already test negative duration and small intervals. So I will add tests with large numbers and saturation. Thanks again.