search

mpusz / mp-units

The quantities and units library for C++
https://mpusz.github.io/mp-units/
MIT License
1.1k stars 90 forks source link

Magnitude constants representation type #631

Open mpusz opened 3 weeks ago

mpusz commented 3 weeks ago

As of today, mag_constant requires a value of long double (e.g., for pi). However, it might not be a good option for embedded projects. Should we change the requirement to just be a floating-point type? How to refactor pi to be embedded-friendly?

JohelEGP commented 3 weeks ago

Make it possible for the mag_constant's value to be parametrized on Rep. Have mp_units::pi carry std::numbers::pi_v rather than a floating-point value.

mpusz commented 3 weeks ago

Yeah, I thought about this as well. But then, how do we define units like:

https://github.com/mpusz/mp-units/blob/2892fd83b0551cd6ccbb63d448b3474e7c1c66b6/src/systems/include/mp-units/systems/si/units.h#L103-L105

Do we want to expose Rep from a unit as well? Note that we need to do it not only for degree, but even for every other unit build on top of a degree (even if it has an integral magnitude relative to degree).

JohelEGP commented 3 weeks ago

Why would those need to change? There's no need to "expose Rep". pi remains a constant in the magnitude expression, and you only ever need to instantiate its value with a representation type when its time to convert to another unit without pi.

chiphogg commented 3 weeks ago

I'm confused: do we have any evidence that the current setup is not embedded friendly?

I haven't kept up on the details of every change to magnitudes since they landed. But my mental model is that the type of the constant never manifests in any runtime-used value. I would expect users to only ask for get_value<T>(m), not the raw constant --- and the result of get_value should be computed, in type T, fully at the time the program is built.

That's one of the nice things about magnitudes. You can get your computations performed in higher-precision long double, without ever needing to support that type in your program!

mpusz commented 3 weeks ago

Good point @chiphogg. I forgot about get_value.

Is long double available on all the embedded platforms? Said otherwise, is it possible to cross-compile the current pi definition to every platform?

mpusz commented 3 weeks ago

Although, long double may appear due to common_magnitude_type being used in sudo_cast.

mpusz commented 3 weeks ago

Why would those need to change? There's no need to "expose Rep". pi remains a constant in the magnitude expression, and you only ever need to instantiate its value with a representation type when its time to convert to another unit without pi.

Thanks, now I understand what you meant.

chiphogg commented 3 weeks ago

Good point @chiphogg. I forgot about get_value.

Is long double available on all the embedded platforms? Said otherwise, is it possible to cross-compile the current pi definition to every platform?

I don't actually know. Empirically, I've never seen a bug report about long double causing problems, although we have had various other issue reports from embedded users, both Aurora-internal and external. So I'm guessing we would have heard about it by now, but I can't be completely sure.