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

Maximize common unit for QuantityPoint #369

Open
chiphogg opened this issue Dec 23, 2024 · 0 comments
Open

Maximize common unit for QuantityPoint #369

chiphogg opened this issue Dec 23, 2024 · 0 comments
Labels
⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: bug Something isn't working correctly, or there's a mistake 📝 status: blocked Blocked on another issue or an external dependency 💪 effort: medium
Milestone

Comments

@chiphogg
Copy link
Contributor

Consider this test, from :quantity_point_test:

TEST(QuantityPoint, CommonPointUnitLabel) {
    EXPECT_THAT(stream_to_string(celsius_pt(0) - kelvins_pt(0)),
                AnyOf(StrEq("5463 EQUIV{[(1 / 20) K], [(1 / 20) degC]}"),
                      StrEq("5463 EQUIV{[(1 / 20) degC], [(1 / 20) K]}")));
}

This passes... but only because we're using non-standard and optimized definitions for Celsius and Kelvins in that file. The actual code on Au 0.4.1 gives "27315 EQUIV{[(1 / 100) degC], [(1 / 100) K]}". Yes, it's a little easier to recognize the correct value, but it's also a factor of 5 bigger, which means unnecessary additional overflow risk.

We could try manually specifying every origin in the most efficient units. However, this is a losing game, because we would need to consider all pairwise combinations of multiple units --- what's going to happen when we bring Fahrenheit into play?

My first attempt to solve this was to store the origin difference in a unit where its value was 1. I was worried this wouldn't be possible at compile time, but since the origins are constexpr quantities, it actually is! The problem with this is that such a big unit hits our overflow safety surface pretty easily. But if it's possible to construct this unit in the first place (and it is), then it should be possible to use a Constant, and take advantage of its perfect conversion policy!

This is what I think we should do. I've done enough exploring to convince myself that this is possible. We're blocked on negative constants, though, and that has a lot of implications we'll need to work through.

@chiphogg chiphogg added 📁 kind: bug Something isn't working correctly, or there's a mistake 💪 effort: medium ⬇️ affects: code (implementation) Affects implementation details of the code labels Dec 23, 2024
@chiphogg chiphogg added this to the On deck milestone Dec 23, 2024
@chiphogg chiphogg added the 📝 status: blocked Blocked on another issue or an external dependency label Dec 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⬇️ affects: code (implementation) Affects implementation details of the code 📁 kind: bug Something isn't working correctly, or there's a mistake 📝 status: blocked Blocked on another issue or an external dependency 💪 effort: medium
Projects
None yet
Development

No branches or pull requests

1 participant