added torque as an alias of energy #124

dunmatt · 2019-04-20T22:14:27Z

After a lot of experimentation I'm coming around to the idea that Moments are more trouble than they're worth. So in that vein, I'm following your suggestion in #117 to not use a different Kind for torque, and to add it as a quantity.

To get that to work I had to refactor the quantity macro a bit to split the parts that must be repeated between aliases from the parts that cannot be. I'm rather new to macros (I still find them very hard to read), so please bear with me if it takes some back and forth to clean these changes up.

coveralls · 2019-04-20T22:37:42Z

Coverage increased (+0.09%) to 97.206% when pulling e09758b on dunmatt:addingTorque into e70b0a4 on iliekturtles:master.

iliekturtles

Thanks for the PR! I did a very quick first pass through the changes and will need to do a more detailed pass this week.

iliekturtles · 2019-04-21T19:06:10Z

src/quantity.rs

@@ -113,6 +113,25 @@ macro_rules! quantity {
            }
        }
    };
+    (
+        $(#[$quantity_attr:meta])* quantity: $quantity:ident; $description:expr;
+        alias_of: $alias_of:ident;


Why add the alias? In my first brief look at the code it looks like it allows the alias's units to be used?

Quantities with the same dimension and kind are already type aliases although I'm they can't share units currently.

type Energy = Quantity<m^2 * kg * s^-2>; type Torque = Quantity<m^2 * kg * s^-2>;

It's so the Conversion can extend the Unit of both the original type and the alias. Or to take it at a deeper level it's because "I don't fully understand how this library hangs together, but I had to do this in order to get test to compile."

iliekturtles · 2019-04-21T19:11:28Z

src/si/mod.rs

@@ -49,6 +49,7 @@ system! {
        temperature_interval::TemperatureInterval,
        thermodynamic_temperature::ThermodynamicTemperature,
        time::Time,
+        torque_magnitude::TorqueMagnitude,


I'm inclined towards Torque instead of TorqueMagnitude. See Force as an existing example. Additionally I'd like to get support for mathematical vectors as the underlying storage type some day (e.g. Quantity<V = Vec3f32> rather than Vec3f32<T = Quantity>). Thoughts?

I've done the renaming. In terms of planning for vectors, I'll spend some time thinking about it and let you know what would be a best case scenario for my uses.

So, I spent some time poking around and it seems like nobody has written the vector library I want, yet. It's very tempting to launch yet another hobby project. What I really want is compile time checking of transform matrix math, to prevent nonsense matrix combinations (ie if Tab transforms a point or vector from a to b, I want Tab*Tbc == Tac and both Tab*Tcb and Tbc*Tab should break the build)... but I don't strictly need them to be quantities, all I really care about is compile time frame of reference checking.

I'm curious though, what's your motivation for wanting a vector quantity (as opposed to a vectory of quantities)?

I figured a Quantity<V = Vec3f32> would be more natural to use than Vec3<T = Quantity>. No actual usage experience and I'm not looking to attempt to implement now unless someone actually has a use case.

src/si/torque_magnitude.rs

iliekturtles · 2019-04-21T19:29:27Z

src/si/torque_magnitude.rs

@@ -0,0 +1,155 @@
+//! Torque (aka moment of force) (base unit newton meter, kg<sup>1</sup> · m<sup>2</sup> ·


List base quantities in the same order as specified in the system! macro: length, mass, time, .... Superscript isn't necessary when equal to 1.

Shall I update Force as well? (that's where I copy pasted from)

Added #127 regarding units. Reply there / submit a new PR if you're interested. Otherwise I'll do in the next few days. We'll also want to make kg superscript consistent with that issue. Leaning towards leaving the 1 right now.

iliekturtles · 2019-04-24T20:17:04Z

Changes in torque.rs look pretty good. Still needs a full detailed review, but I don't expect that to take long. Also need to fully understand the alias_of changes. I'm hoping to get to all of this over the weekend.

iliekturtles · 2019-04-28T13:51:58Z

Finally got to look at this with the compiler and I understand what is happening now. Energy and Torque are both aliases for Quantity<Dimension<P2, P1, N2>, U, V>. The quantity! macro implements FromStr which causes a conflicting implementations error shown below.

A short-term solution could be to revert the alias_of changes in quantity.rs and give Torque a kind so that it becomes distinct from Energy. A From<Energy> for Torque implementation would likely also be needed. I don't think this is the right long-term solution but we could do it right now and include Torque in the next uom release. Once a better long-term solution is found the kind could be dropped.

Another possible solution would be to change the FromStr implementation to return a type parameterized on the quantity's Unit trait so that parsing would be something like let t: Torque = "1 N".parse::<(Torque, torque::Unit)>().into(). I don't think this exact form would work so more experimentation is needed.

error[E0119]: conflicting implementations of trait `std::str::FromStr` for type `si::Quantity<(dyn si::D
imension<M=typenum::PInt<typenum::UInt<typenum::UTerm, typenum::B1>>, J=typenum::Z0, L=typenum::PInt<typ
enum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>, T=typenum::NInt<typenum::UInt<type
num::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>, Th=typenum::Z0, N=typenum::Z0, Kind=(dyn Kind + '
static), I=typenum::Z0> + 'static), _, f32>`:
   --> src\quantity.rs:348:17
    |
348 |                   impl<U> FromStr for super::super::$quantity<U, V>
    |  _________________^
    | |_________________|
    | |
349 | |                 where
350 | |                     U: super::super::super::Units<V> + ?Sized,
351 | |                 {
...   |
364 | |                     }
365 | |                 }
    | |                 ^
    | |_________________|
    | |_________________first implementation here
    |                   conflicting implementation for `si::Quantity<(dyn si::Dimension<M=typenum::PInt<
typenum::UInt<typenum::UTerm, typenum::B1>>, J=typenum::Z0, L=typenum::PInt<typenum::UInt<typenum::UInt<
typenum::UTerm, typenum::B1>, typenum::B0>>, T=typenum::NInt<typenum::UInt<typenum::UInt<typenum::UTerm,
 typenum::B1>, typenum::B0>>, Th=typenum::Z0, N=typenum::Z0, Kind=(dyn Kind + 'static), I=typenum::Z0> +
 'static), _, f32>`
    |
   ::: src\si\torque.rs:3:1
    |
3   | / quantity! {
4   | |     /// Torque magnitude (base unit newton meter, kg<sup>1</sup> · m<sup>2</sup> · s<sup>-2</sup
>).
5   | |     ///
6   | |     /// Torques are moments, which means they inherently depend on a distance to a frame of
...   |
75  | |     }
76  | | }
    | |_- in this macro invocation

dunmatt · 2019-04-28T18:23:17Z

At least for my edification, what's the problem with the way it's implemented now? Just that things can't be parsed into torques?

iliekturtles · 2019-04-28T19:52:57Z

With the current alias_of code Torque::from_str will only parse Energy abbreviations and use those conversion factors. Some other methods like new, get, `floor, ... look like they are only implemented for the parent and not the alias, although I didn't test.

We need to find some way to separate Energy and Torque from_str implementations so they can each use their own unit definitions. Finding some way for the two to share units or be sub/super-sets of each other is another option and then share a from_str implementation.

dunmatt · 2019-04-29T18:15:16Z

How would you feel about a blanket From implementation that can switch between Kinds?

iliekturtles · 2019-04-29T20:13:40Z

A blanket implementation probably makes sense. I've been thinking about this a little bit today and I'm not sure if it makes sense to ever have different quantities, even if they are the same dimension to be strict aliases of themselves.

let e: Energy = ...;

energy_fn(e);
torque_fn(e);

fn energy_fn(e: Energy) {}
fn torque_fn(t: Torque) {}

The above works without any warnings and could easily lead to mixing up quantities of the same dimension. A blanket implementation would at least require a .into() telling the user a type change was happening.

My thoughts aren't fully formed yet, but likely more design effort needs to be put into what is called kind right now.

dunmatt · 2019-04-30T16:06:00Z

I'm working at the edge of my skill here, can you sanity check this?

    impl<K, U, V> From<super::Quantity<super::$system<$($crate::typenum::$dimension),+, K>, U, V>> for super::Quantity<Dimension, U, V>
    where
        U: super::Units<V>,
        V: num_traits::Num + $crate::Conversion<V>,
    {
        fn from(val: super::Quantity<super::$system<$($crate::typenum::$dimension),+, K>, U, V>) -> super::Quantity<Dimension, U, V> {
            unimplemented!()
        }
    }

Edit: I'm running into some problems due to the lack of things being Sized:

error[E0277]: the size for values of type dyn Kind cannot be known at compilation time
--> src/si/torque.rs:150:79
|
150 | &(Force::new::(V::one()) * Length::new::(V::one())).into());
| ^^^^ doesn't have a size known at compile-time
|
= help: the trait std::marker::Sized is not implemented for dyn Kind
= note: to learn more, visit https://doc.rust-lang.org/book/second-edition/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait
= note: required because of the requirements on the impl of std::convert::From<si::Quantity<dyn si::Dimension<T=typenum::NInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>, I=typenum::Z0, N=typenum::Z0, Kind=dyn Kind, M=typenum::PInt<typenum::UInt<typenum::UTerm, typenum::B1>>, J=typenum::Z0, Th=typenum::Z0, L=typenum::PInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>>, dyn si::Units<f32, amount_of_substance=si::amount_of_substance::mole, electric_current=si::electric_current::ampere, time=si::time::second, thermodynamic_temperature=si::thermodynamic_temperature::kelvin, mass=si::mass::kilogram, length=si::length::meter, luminous_intensity=si::luminous_intensity::candela>, f32>> for si::Quantity<dyn si::Dimension<T=typenum::NInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>, I=typenum::Z0, N=typenum::Z0, Kind=dyn si::angle::AngleKind, M=typenum::PInt<typenum::UInt<typenum::UTerm, typenum::B1>>, J=typenum::Z0, Th=typenum::Z0, L=typenum::PInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>>, dyn si::Units<f32, amount_of_substance=si::amount_of_substance::mole, electric_current=si::electric_current::ampere, time=si::time::second, thermodynamic_temperature=si::thermodynamic_temperature::kelvin, mass=si::mass::kilogram, length=si::length::meter, luminous_intensity=si::luminous_intensity::candela>, f32>
= note: required because of the requirements on the impl of std::convert::Into<si::Quantity<dyn si::Dimension<T=typenum::NInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>, I=typenum::Z0, N=typenum::Z0, Kind=dyn si::angle::AngleKind, M=typenum::PInt<typenum::UInt<typenum::UTerm, typenum::B1>>, J=typenum::Z0, Th=typenum::Z0, L=typenum::PInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>>, dyn si::Units<f32, amount_of_substance=si::amount_of_substance::mole, electric_current=si::electric_current::ampere, time=si::time::second, thermodynamic_temperature=si::thermodynamic_temperature::kelvin, mass=si::mass::kilogram, length=si::length::meter, luminous_intensity=si::luminous_intensity::candela>, f32>> for si::Quantity<dyn si::Dimension<T=typenum::NInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>, I=typenum::Z0, N=typenum::Z0, Kind=dyn Kind, M=typenum::PInt<typenum::UInt<typenum::UTerm, typenum::B1>>, J=typenum::Z0, Th=typenum::Z0, L=typenum::PInt<typenum::UInt<typenum::UInt<typenum::UTerm, typenum::B1>, typenum::B0>>>, dyn si::Units<f32, amount_of_substance=si::amount_of_substance::mole, electric_current=si::electric_current::ampere, time=si::time::second, thermodynamic_temperature=si::thermodynamic_temperature::kelvin, mass=si::mass::kilogram, length=si::length::meter, luminous_intensity=si::luminous_intensity::candela>, f32>

error[E0277]: the size for values of type dyn si::Units<f32, amount_of_substance=si::amount_of_substance::mole, electric_current=si::electric_current::ampere, time=si::time::second, thermodynamic_temperature=si::thermodynamic_temperature::kelvin, mass=si::mass::kilogram, length=si::length::meter, luminous_intensity=si::luminous_intensity::candela> cannot be known at compilation time
--> src/si/torque.rs:150:79
|
150 | &(Force::new::(V::one()) * Length::new::(V::one())).into());
| ^^^^ doesn't have a size known at compile-time
|

Any ideas / suggestions? Why does uom use ?Sized everywhere?

iliekturtles · 2019-05-01T02:08:17Z

Sat down to look at this and realized a blanket impl isn't possible. See #58. However an explicit into() method can be implemented on Quantity. I tested the following in the impl Quantity<D, U, V> in system.rs. Name conflicts with Into::into in other tests and I didn't take time to figure the workaround yet so it's called iinto right now. Uncommenting the second into() call causes the test to fail.

            #[cfg_attr(all(feature = "si", feature = "f32"), doc = " ```rust")]
            #[cfg_attr(not(all(feature = "si", feature = "f32")), doc = " ```rust,ignore")]
            /// # use uom::si::f32::*;
            /// # use uom::si::thermodynamic_temperature::kelvin;
            /// let tt: ThermodynamicTemperature = ThermodynamicTemperature::new::<kelvin>(1.0);
            /// let ti: TemperatureInterval = tt.iinto();
            /// //let f: Frequency = ti.into();
            /// ```
            #[inline(always)]
            pub fn iinto<Dr, Ur>(self) -> Quantity<Dr, Ur, V>
            where
                $(D::$symbol: $crate::lib::ops::IsEqual<Dr::$symbol>,)+
                Dr: Dimension + ?Sized,
                Ur: Units<V> + ?Sized,
            {
                Quantity {
                    dimension: $crate::lib::marker::PhantomData,
                    units: $crate::lib::marker::PhantomData,
                    value: change_base::<Dr, U, Ur, V>(&self.value),
                }
            }

dunmatt · 2019-05-01T16:10:26Z

I don't understand what #58 has to do with this change. My From impl above compiles just fine, the problems only arise at the call site because the compiler doesn't know what size anything is. Can you help me understand the core of the issue?

Is there a way I could solve the specific problem to allow the Torque tests to work so that I can get those in (and start using them) and worry about the more general case later?

iliekturtles · 2019-05-02T00:16:04Z

Regarding #58 I was thinking about a blanket impl in the system! macro that would apply to all quantities. This isn't possible because of the overlapping impl<T> From<T> for T in the standard library. Doing the impl in the quantity! macro like you are doing may get around this. If you add a K: ?Sized constraint to your impl can you compile or do you get the same conflicting implementations error?

You should also add two base units parameters (Ul, Ur) so that From can be used to change kinds or base units.

I just skimmed the code so far but will plan to look more in depth tomorrow evening.

dunmatt · 2019-05-02T16:03:10Z

Ok, so adding K: ?Sized, got me to the point where I'm now having the conflicting implementations problem you're talking about. As a potential solution, what if I made a new pattern for the quantity macro that did the standard thing, and then also created From impls that convert things between Kind and AngleKind? So since the Kinds would be fully specified (and different) it wouldn't conflict with the blanket implementation in std.

As to your Ul and Ur idea. I don't think it's a good idea to allow converting both of those things as once, if I'm using into to create an AngleKind of a Length, I don't want to be able to screw up and accidentally turn an Area into an AngleKind of Length. If there was a way to have an implementation do one or the other but not both that might be okay, but I don't know how that might be achieved.

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

I was afraid you would get the same conflicting implementation error. Explicitly specifying Kinds is probably going to be needed. Let us know how that works.

For changing base units Ul to Ur this doesn't affect dimension, only the base units a quantity is stored in. e.g. Length<U<meter>> to Length<U<foot>> (not actual syntax). Go ahead and implement with a single base unit, U, first and then we can see if it makes sense to support changing.

…turtles#117 (and finishes the request in iliekturtles#114).

M@ Dunlap added 2 commits April 20, 2019 15:07

added torque as an alias of energy

e6e875f

minor cleanups

b05efe4

moved the fmt arguments out of common to hopefully fix CI

47ccff8

iliekturtles requested changes Apr 21, 2019

View reviewed changes

M@ Dunlap added 2 commits April 21, 2019 13:43

addressed first round of PR feedback

39e0397

removed extra spaces per feedback in another PR

e09758b

dunmatt mentioned this pull request May 3, 2019

added Torque #132

Merged

dunmatt pushed a commit to dunmatt/uom that referenced this pull request May 8, 2019

Added Torque to resolve iliekturtles#132, iliekturtles#124, and iliek…

cf9af7d

…turtles#117 (and finishes the request in iliekturtles#114).

dunmatt closed this May 8, 2019

dunmatt deleted the addingTorque branch May 8, 2019 20:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added torque as an alias of energy #124

added torque as an alias of energy #124

dunmatt commented Apr 20, 2019

coveralls commented Apr 20, 2019 •

edited

Loading

iliekturtles left a comment

iliekturtles Apr 21, 2019

dunmatt Apr 21, 2019 •

edited

Loading

iliekturtles Apr 21, 2019

dunmatt Apr 21, 2019

dunmatt Apr 26, 2019

iliekturtles Apr 27, 2019

iliekturtles Apr 21, 2019

dunmatt Apr 21, 2019

iliekturtles Apr 24, 2019

iliekturtles commented Apr 24, 2019

iliekturtles commented Apr 28, 2019

dunmatt commented Apr 28, 2019 •

edited

Loading

iliekturtles commented Apr 28, 2019

dunmatt commented Apr 29, 2019 •

edited

Loading

iliekturtles commented Apr 29, 2019

dunmatt commented Apr 30, 2019 •

edited

Loading

iliekturtles commented May 1, 2019

dunmatt commented May 1, 2019

iliekturtles commented May 2, 2019

dunmatt commented May 2, 2019

iliekturtles commented May 3, 2019

		@@ -0,0 +1,155 @@
		//! Torque (aka moment of force) (base unit newton meter, kg<sup>1</sup> · m<sup>2</sup> ·

added torque as an alias of energy #124

added torque as an alias of energy #124

Conversation

dunmatt commented Apr 20, 2019

coveralls commented Apr 20, 2019 • edited Loading

iliekturtles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunmatt Apr 21, 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

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

iliekturtles commented Apr 24, 2019

iliekturtles commented Apr 28, 2019

dunmatt commented Apr 28, 2019 • edited Loading

iliekturtles commented Apr 28, 2019

dunmatt commented Apr 29, 2019 • edited Loading

iliekturtles commented Apr 29, 2019

dunmatt commented Apr 30, 2019 • edited Loading

iliekturtles commented May 1, 2019

dunmatt commented May 1, 2019

iliekturtles commented May 2, 2019

dunmatt commented May 2, 2019

iliekturtles commented May 3, 2019

coveralls commented Apr 20, 2019 •

edited

Loading

dunmatt Apr 21, 2019 •

edited

Loading

dunmatt commented Apr 28, 2019 •

edited

Loading

dunmatt commented Apr 29, 2019 •

edited

Loading

dunmatt commented Apr 30, 2019 •

edited

Loading