mp-units icon indicating copy to clipboard operation
mp-units copied to clipboard

SI Torque unit

Open volkerneff opened this issue 2 years ago • 7 comments

Hello all,

First of all, I would like to say that this is a very practical and powerful library that will hopefully save us many bugs in the future.

However, I stumbled upon the SI Torque unit. It is defined in the library as Nm/rad. However, if you look at the Wikipedia entry for torque, the SI unit there is Nm. The angle only comes into play when it is a force vector, but not for a single scalar value.

What was the reason for choosing a unit like this and is it possible to change it?

Thank you very much for your support,

volkerneff avatar May 01 '22 20:05 volkerneff

Hi, this is a long story. You can read more in #32 and #99. Please see also this https://github.com/mpusz/units/issues/239#issuecomment-784559683.

Please let me know if you have some ideas for improving it.

mpusz avatar May 05 '22 08:05 mpusz

Thanks, Mateusz for the links. I have read them as well as I could and tried to follow them. I have the following suggestion: The alias types are into an inline namespace. What's wrong with doing that for the Si types as well? The compiler would find the appropriate implementation via the inline namespace if only one file is included. If there are conflicting implementations included, you could simply specify the namespace explicitly. This also prevents unwanted explicit conversion. If it is useful for some types you could implement explicit cast or implicit systems.

I hope this is not too short, because I had a hard time getting started with the library, despite the excellent documentation.

namespace Foo::inline a {
    class Bar {
    };
}

namespace Foo::inline b {
    class Bar {
    };
}

int main() {
    Foo::a::Bar barA;
    Foo::b::Bar barB;
    Foo::Bar bar; // undefined but clear error
    /*
    GCC 12.1
    <source>: In function 'int main()':
    <source>:21:10: error: reference to 'Bar' is ambiguous
    21 |     Foo::Bar bar; // undefined but clear error
        |          ^~~
    <source>:9:11: note: candidates are: 'class Foo::a::Bar'
        9 |     class Bar {
        |           ^~~
    <source>:14:11: note:                 'class Foo::b::Bar'
    14 |     class Bar {
        |   
    */

    /*
        Foo::Bar bar; // undefined but clear error
             ^
    <source>:9:11: note: candidate found by name lookup is 'Foo::a::Bar'
        class Bar {
            ^
    <source>:14:11: note: candidate found by name lookup is 'Foo::b::Bar'
        class Bar {
    */
}

volkerneff avatar May 16 '22 19:05 volkerneff

I am not sure if I fully understand what you mean. Do you recommend creating inline namespaces in the SI for dimensions and putting all definitions there? For example:

namespace units::isq::si::inline speed {

struct metre_per_second : derived_unit<metre_per_second> {};
struct dim_speed : isq::dim_speed<dim_speed, metre_per_second, dim_length, dim_time> {};

struct kilometre_per_hour : derived_scaled_unit<kilometre_per_hour, dim_speed, kilometre, hour> {};

template<UnitOf<dim_speed> U, Representation Rep = double>
using speed = quantity<dim_speed, U, Rep>;

}

Please note that in such a case we end up with units::isq::si::speed::speed<U, Rep> which looks wrong. Also skipping the namespace part could be impossible because of this naming collision (the compiler will be confused if you meant inline namespace or the alias).

mpusz avatar May 17 '22 13:05 mpusz

Yes, you understand me correctly.

Personally, I didn't see a problem with the units::isq::si::speed::speed<U, Rep> because it is only necessary if there are conflicting implementations included.

the compiler will be confused if you meant inline namespace or the alias

I didn't see your point here. For example, the torque header is based on version 0.7. Where is the point where the compiler could have problems?

namespace units::isq::si::inline torque {

struct newton_metre_per_radian : derived_unit<newton_metre_per_radian> {};

struct dim_torque : isq::dim_torque<dim_torque, newton_metre_per_radian, dim_force, dim_length, dim_angle<>> {};

template<UnitOf<dim_torque> U, Representation Rep = double>
using torque = quantity<dim_torque, U, Rep>;

#ifndef UNITS_NO_LITERALS
// ....
#endif  // UNITS_NO_LITERALS

}  // namespace units::isq::si::torque 

#ifndef UNITS_NO_ALIASES

namespace units::aliases::isq::si::inline torque {

template<Representation Rep = double>
using N_m_per_rad = units::isq::si::torque::torque<units::isq::si::newton_metre_per_radian, Rep>;

}  // namespace units::aliases::isq::si::inline torque

#endif  // UNITS_NO_ALIASES

volkerneff avatar May 17 '22 15:05 volkerneff

I copy-pasted your proposed implementation here: https://godbolt.org/z/7G9jMT48E. See in the link how both t1 and t2 produce an error:

using namespace units::isq::si;
torque<newton_metre_per_radian> t1(123);
torque::torque<newton_metre_per_radian> t2(123);

mpusz avatar May 18 '22 13:05 mpusz

Hi, it has been a while 😉 Do you still find the unit of torque confusing, or maybe were you able to make it work for you?

Anyway, today I updated the documentation with the angular quantities chapter: https://mpusz.github.io/units/framework/angular_units.html. Please let me know how it compares to your experience.

mpusz avatar Sep 02 '22 18:09 mpusz

Also, please let me know if we can close this issue.

mpusz avatar Sep 02 '22 18:09 mpusz

Done in V2.

mpusz avatar Jun 15 '23 07:06 mpusz