Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Magnitude constants representation type #631

Open
mpusz opened this issue Nov 5, 2024 · 8 comments
Open

Magnitude constants representation type #631

mpusz opened this issue Nov 5, 2024 · 8 comments
Labels
enhancement New feature or request

Comments

@mpusz
Copy link
Owner

mpusz commented Nov 5, 2024

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?

@mpusz mpusz added the enhancement New feature or request label Nov 5, 2024
@JohelEGP
Copy link
Collaborator

JohelEGP commented Nov 5, 2024

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
Copy link
Owner Author

mpusz commented Nov 5, 2024

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

inline constexpr struct degree final : named_unit<symbol_text{u8"°", "deg"}, mag<π> / mag<180> * si::radian> {} degree;
inline constexpr struct arcminute final : named_unit<symbol_text{u8"", "'"}, mag_ratio<1, 60> * degree> {} arcminute;
inline constexpr struct arcsecond final : named_unit<symbol_text{u8"", "''"}, mag_ratio<1, 60> * arcminute> {} arcsecond;

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
Copy link
Collaborator

JohelEGP commented Nov 5, 2024

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
Copy link
Collaborator

chiphogg commented Nov 5, 2024

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
Copy link
Owner Author

mpusz commented Nov 5, 2024

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
Copy link
Owner Author

mpusz commented Nov 5, 2024

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

@mpusz
Copy link
Owner Author

mpusz commented Nov 5, 2024

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
Copy link
Collaborator

chiphogg commented Nov 5, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants