Added an implementation of From<Time> for Duration #131

dunmatt · 2019-05-01T17:53:47Z

This resolves a request in #58 (comment)

iliekturtles · 2019-05-01T18:05:26Z

Thanks for the PR! I'm planning to review tonight along with responding to the other open conversations.

coveralls · 2019-05-01T18:20:45Z

Coverage increased (+0.02%) to 97.253% when pulling 8493f01 on dunmatt:fromTimeToTime into 8c5eb43 on iliekturtles:master.

dunmatt · 2019-05-01T18:45:46Z

Trying to write a test for this I'm running into the same compile time ?Sized problem as in the Torque PR. Is there a problem with my test?

    #[cfg(all(test, feature = "std"))]
    mod tests {
        storage_types! {
            types: Float;

            use ::lib::time::Duration;
            use si::quantities::*;
            use si::time::second;
            use tests::Test;

            quickcheck! {
                // #[allow(trivial_casts)]
                fn from(v: V) -> bool {
                    let t = Time::new::<second>(v);
                    let d: Duration = t.into();
                    Test::eq(v as u64, d.secs)
                    // Test::eq(&l.hypot(r),
                    //     &Length::new::<meter>(l).hypot(Length::new::<meter>(r)).get::<meter>())
                }
            }
        }
    }

iliekturtles · 2019-05-01T18:48:17Z

Units<V> also needs the ?Sized constraint:

uom/src/quantity.rs

Line 278 in 8c5eb43

U: super::Units<V> + ?Sized,

iliekturtles

Couple comments in the diff.

Did the + ?Sized addition get your tests working?

iliekturtles · 2019-05-01T23:58:08Z

src/si/time.rs

+    V: ::num::Num + ::num::AsPrimitive<f64> + ::Conversion<V>,
+{
+    fn from(t: Time<U, V>) -> Duration {
+        let secs = t.value.as_().abs();


t.value isn't guaranteed to be in seconds (see base.rs). from_base<D, U, V, N>(v) can be used to convert from the Quantity's base units into a specific unit (e.g. seconds).

As a slight aside from_base calculates the result using the underlying storage type's conversion factor type and then converts back to the underlying storage type before returning. For f32/f64 this doesn't make a difference since the underlying storage type and it's conversion factor type are the same. For integer types their conversion factor type is Ratio. I'm wondering if it would make sense to have from_base and related methods (or similar helper methods) return the conversion factor type which could allow this method to do a more precise conversion to Duration (e.g. Ratio<V> -> u64 instead of V -> u64).

I also tried the obvious thing:

fn from(t: Time<U, V>) -> Duration { let secs = t.get::<second>(); let nanos = t.get::<nanosecond>(); // NOTE: needs a modulo 1e9 here Duration::new(secs as u64, nanos as u32) }

but I haven't yet found a way to get it to compile. TBH I'm pretty frustrated with how hard even basic things are. I try to do a thing, and then I invariably do it wrong because I can't make sense of the current code. I tried calling from_base, but there were enough unsatisfied trait bound errors that my brain kinda switched off.

It might just be that I'm not good enough to contribute here, because this conversion ought to be the simplest thing in the world to implement.

iliekturtles · 2019-05-02T00:01:17Z

src/si/time.rs

+{
+    fn from(t: Time<U, V>) -> Duration {
+        let secs = t.value.as_().abs();
+        let nanos = (secs * 1e9) as u64 % 1e9 as u64;


I'm worried about overflow here. It would be good to find a way to extract the nanoseconds without multiplication. Does converting the original value into nanoseconds and then extracting the portion needed for Duration work?

Converting the original value to nanoseconds is a multiplication operation... there's no way around that part. Taking off the integer part before multiplying can solve any potential overflow problems: let nanos = secs.fract() * 1e9;, but it doesn't solve any of the base problems in your other point...

iliekturtles · 2019-05-03T00:20:50Z

It's been almost three years since I started the project and I still spend an excessive amount of time staring at compiler errors trying to figure things out so you're definitely not alone.

I got the code compiling, but it doesn't pass tests. Now to find a correct, and hopefully efficient, algorithm to convert into a Duration!

I was hoping to delay the conversion to u64/u32 to as late as possible and not go through an intermediate type (f64) if possible. This may require multiple implementations via storage_types! in order to satisfy the fractional/integer and signed/unsigned differences. It may also be a better use of time to land the simplest algorithm now.
I added a modulo prior to the nanosecond conversion. Multiplication avoided! Not sure what other issues this introduces. Precision issues?
Test doesn't seem to require std so I removed.
It would be good to see if this could be made into a quickcheck test. Maybe use the Duration::from_nanos constructor and compare against Time::<nanosecond>::new(v).into()?
Testing with different base units would be good but I'm not sure what kind of compile time hit it would be calling the ISQ! macro a couple times.

diff --git a/src/si/time.rs b/src/si/time.rs
index 4f267a2..b6fbd8f 100644
--- a/src/si/time.rs
+++ b/src/si/time.rs
@@ -50,16 +50,19 @@ quantity! {
 impl<U, V> From<Time<U, V>> for Duration
 where
     U: ::si::Units<V> + ?Sized,
-    V: ::num::Num + ::num::AsPrimitive<f64> + ::Conversion<V>,
+    V: ::num::Num + ::num::AsPrimitive<u64> + ::num::AsPrimitive<u32> + ::Conversion<V>,
+    second: ::Conversion<V, T = V::T>,
+    nanosecond: ::Conversion<V, T = V::T>,
 {
     fn from(t: Time<U, V>) -> Duration {
-        let secs = t.value.as_().abs();
-        let nanos = (secs * 1e9) as u64 % 1e9 as u64;
-        Duration::new(secs as u64, nanos as u32)
+        let secs = t.get::<second>().as_();
+        let nanos = (t % Time::<U, V>::new::<second>(V::one())).get::<nanosecond>().as_();
+
+        Duration::new(secs, nanos)
     }
 }

-#[cfg(all(test, feature = "std"))]
+#[cfg(test)]
 mod tests {
     storage_types! {
         types: Float;
@@ -72,8 +75,9 @@ mod tests {
         fn from() {
             let t = Time::new::<second>(-21.5);
             let d: Duration = t.into();
+
             assert_eq!(21, d.as_secs());
-            assert_eq!(5e8 as u32, d.subsec_nanos());
+            assert_eq!(500_000_000, d.subsec_nanos());
         }
     }
 }

dunmatt · 2019-05-03T02:26:34Z

Resuming work, the reason your test failed was because you didn't call t.abs(). The trouble I'm working on now is that abs is a method on num_traits::Signed, and for whatever reason num_traits::Signed and num_traits::Unsigned are not disjoint (or perhaps the compiler can't prove they are), so you get conflicting impls if you naively try to only call abs when it's present.

Your "use quickcheck" idea is why the tests module was gated on std; I was copying from Length (but took out quickcheck during the compiler struggles).

Aehmlo · 2019-05-03T05:50:15Z

src/si/time.rs

-            secs = V::zero() - secs;
-        }
-        let secs = secs.as_();
+        let t = if t < Time::<U, V>::new::<second>(V::zero()) {


I understand why this is here, but do we want to silently convert negative times to (inherently positive) durations? I suppose this might be a matter of "time vs. time interval."

I don't think it's really a silent conversion, the user has to manually call into in a context where the conversion is into an inherently positive type. I guess it's a question of which is least surprising:

Absolute Value

Wrapping Around

Minimum 0

panic!

I know that I would prefer panicking, although I’d imagine that others would see it differently.

Really? I just put that option there for completeness, From must not fail

In the context of that documentation page, "fallible functions" seem to be loosely be "functions that return Result<T, E> or Option<T>:

When constructing a function that is capable of failing the return type will generally be of the form Result<T, E>.

As such, I read that note as simply meaning "From does not return an error type." A more general argument for this interpretation is that it's almost nonsensical to say "this code must not panic" (with a couple of notable exceptions) in Safe Rust.

Panics signal an irrecoverable error, something that makes no sense and for which there's no sensible alternative route. In my mind, converting a negative time to a Duration is such an error, and it makes sense to panic, as this likely signals a logic error. However, panicking from within a library crate is certainly not ideal.

I actually might argue, if possible, that we should use TryFrom instead of From.

So TryFrom vs From is a different issue, there's certainly an argument to be made there. The trick is, doing that sort of error management (while certainly more rigorous) is enough of an ergonomics hit that if that's the direction uom decides to go I'm going to just continue defining my own conversion functions where and as I need them. I'm not saying you're wrong, you're not, TryFrom is definitely a more correct fit (even setting aside the absolute value question, NaN is sufficient to justify TryFrom), it just feels cumbersome (and the implementation is going to be even more of a nightmare).

So with that out of the way, I think your reading of the note is wrong, and here's why. There's absolutely no point saying "From does not return an error type", because there is no flexibility in from's return type. There's simply no choice what you return. It would be warning us to make sure our code builds, or warning us to not write bugs into our code.

If you assume they added (and emphasized) that warning for a reason, there must be more to it than that. It must be more general than "don't do what you can't do". The reason they warn us not to let it fail is because From is integral to Rust's failure handling mechanisms, and it's not at all clear what should happen when failure handling fails.

... so... I think I just accidentally convinced myself that this PR should be rejected on account of NaN and the infinities. sigh

Not to beat a dead horse, but I still believe that the meaning for that documentation for From is simply "the return value is not an error type." This is especially supported by the fact that you can impl From<A> for Option<B> to mimic TryFrom (and so there is indeed some flexibility in the return type). Regardless, I do think that TryFrom is the better fit here. May I ask why From is necessary for your use case/why TryFrom is insufficient?

It's not that the trait is insufficient on its own (I mean, it's a bit cumbersome to have to do yet more error handling, but at the end of the day that's not that big a deal), it's more that the difficulty of making the change is greater than the amount I care about it being made. I've already sunk 100x more time into this PR than its actually worth to me, and if I were to double that investment I still don't think that's enough to adequately implement TryFrom.

That makes sense, thanks. I think I might try to make up a TryFrom PR, fwiw. Thanks for your efforts on this.

Aehmlo · 2019-05-03T05:51:07Z

src/si/time.rs

-            assert_eq!(21, d.as_secs());
-            // assert_eq!(5e8 as u32, d.subsec_nanos());
+            assert_eq!(d.as_secs(), 21);
+            assert!(499_999_999 <= d.subsec_nanos() && d.subsec_nanos() <= 500_000_001);


Is there a reason that we can't require this to be exactly 500,000,000?

Precision loss when I subtract t from zero is my current best guess. The way I implemented it originally, by casting the time to f64 and then converting that to a Duration did allow this to be 500,000,000 exactly. Perhaps it's only the f32 test that loses precision?

Resolves #131 and #150.

Functionality is not implemented for `Ratio` types because ToPrimitive is not implemented. Resolves #131 and #150.

Added an implementation of From<Time> for Duration.

5bb09d5

dunmatt force-pushed the fromTimeToTime branch from e6dd717 to 5bb09d5 Compare May 1, 2019 18:06

M@ Dunlap added 2 commits May 1, 2019 12:22

added a test for From

258ecb5

added a call to abs since Durations cannot be negative

7515865

iliekturtles requested changes May 2, 2019

View reviewed changes

M@ Dunlap added 2 commits May 2, 2019 19:48

wat wat wat wat wat wat wat

14bcc35

nightmare complete, the test passes

9755822

Aehmlo reviewed May 3, 2019

View reviewed changes

M@ Dunlap added 4 commits May 3, 2019 08:00

added another test case to fix code coverage

3e3969f

widened the acceptable range on the new test

9caa47b

added another test case to maybe fix code coverage?

82d6e99

refactored from to remove else to hopefully fix code coverage

8493f01

iliekturtles pushed a commit that referenced this pull request Sep 4, 2019

Implement fallible Time/Duration conversions.

97a27b5

Resolves #131 and #150.

iliekturtles pushed a commit that referenced this pull request Sep 8, 2019

Implement fallible Time/Duration conversions.

0fd8728

Resolves #131 and #150.

iliekturtles pushed a commit that referenced this pull request Sep 9, 2019

Implement fallible Time/Duration conversions.

3e41b55

Resolves #131 and #150.

iliekturtles pushed a commit that referenced this pull request Sep 9, 2019

Implement fallible Time/Duration conversions.

349db4b

Resolves #131 and #150.

iliekturtles pushed a commit that referenced this pull request Sep 10, 2019

Implement fallible Time/Duration conversions.

8b047bf

Resolves #131 and #150.

iliekturtles pushed a commit that referenced this pull request Sep 13, 2019

Implement fallible Time/Duration conversions.

d8e8306

Functionality is not implemented for `Ratio` types because ToPrimitive is not implemented. Resolves #131 and #150.

iliekturtles closed this in 40ed213 Sep 18, 2019

iliekturtles mentioned this pull request Sep 18, 2019

Implement TryFrom<Time> for Duration #150

Closed

iliekturtles pushed a commit that referenced this pull request Sep 18, 2019

Implement fallible Time/Duration conversions.

a53a65f

Functionality is not implemented for `Ratio` types because ToPrimitive is not implemented. Resolves #131 and #150.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added an implementation of From<Time> for Duration #131

Added an implementation of From<Time> for Duration #131

dunmatt commented May 1, 2019

iliekturtles commented May 1, 2019

coveralls commented May 1, 2019 •

edited

Loading

dunmatt commented May 1, 2019 •

edited

Loading

iliekturtles commented May 1, 2019

iliekturtles left a comment

iliekturtles May 1, 2019

dunmatt May 2, 2019 •

edited

Loading

iliekturtles May 2, 2019

dunmatt May 2, 2019 •

edited

Loading

iliekturtles commented May 3, 2019

dunmatt commented May 3, 2019 •

edited

Loading

Aehmlo May 3, 2019

dunmatt May 3, 2019 •

edited

Loading

Aehmlo May 3, 2019

dunmatt May 3, 2019

Aehmlo May 3, 2019

dunmatt May 4, 2019 •

edited

Loading

dunmatt May 4, 2019 •

edited

Loading

Aehmlo May 8, 2019

dunmatt May 8, 2019 •

edited

Loading

Aehmlo May 8, 2019

Aehmlo May 3, 2019

dunmatt May 3, 2019

Added an implementation of From<Time> for Duration #131

Added an implementation of From<Time> for Duration #131

Conversation

dunmatt commented May 1, 2019

iliekturtles commented May 1, 2019

coveralls commented May 1, 2019 • edited Loading

dunmatt commented May 1, 2019 • edited Loading

iliekturtles commented May 1, 2019

iliekturtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt May 2, 2019 • edited Loading

Choose a reason for hiding this comment

iliekturtles commented May 3, 2019

dunmatt commented May 3, 2019 • edited Loading

Choose a reason for hiding this comment

dunmatt May 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt May 4, 2019 • edited Loading

Choose a reason for hiding this comment

dunmatt May 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt May 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 1, 2019 •

edited

Loading

dunmatt commented May 1, 2019 •

edited

Loading

dunmatt May 2, 2019 •

edited

Loading

dunmatt May 2, 2019 •

edited

Loading

dunmatt commented May 3, 2019 •

edited

Loading

dunmatt May 3, 2019 •

edited

Loading

dunmatt May 4, 2019 •

edited

Loading

dunmatt May 4, 2019 •

edited

Loading

dunmatt May 8, 2019 •

edited

Loading