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

percent_t FROM double and TO double are different #301

Open
esppat opened this issue Nov 25, 2022 · 9 comments
Open

percent_t FROM double and TO double are different #301

esppat opened this issue Nov 25, 2022 · 9 comments

Comments

@esppat
Copy link

esppat commented Nov 25, 2022

Compiling on Linux with up-to-date version at bug date

Consider this code:

double d50 = 50.0;
percent_t p50 = d50;
double d50_2 = p50;

Here, d50_2 value is 0.5, which is a problem.
If assigning a double to a percent_t considers the double holds PERCENT, then the oposite operation percent_t assigned to double should give PERCENT and not absolute value.

Am I wrong?

@esppat
Copy link
Author

esppat commented Nov 25, 2022

I saw old closed tickets where Nick (thanks to him for this great job) says the way percent_t works is by design, but I disagree, this is simply inconsistency.

@nholthaus
Copy link
Owner

nholthaus commented Nov 29, 2022

You're not wrong. I recently wrestled with trying to make this consistent in v3.x and started seeing all the warts in concentration units.

I think that the problem goes back to the fact that "dimensionless" units really lack context for conversions, and so unlike the rest of the dimensioned units it's not possible to squeeze out all the features while still being compatible with a double conversion.

So in your example, you're totally right, one would expect identity. But consider this case:

percent_t p50 = 50.0;
meter_t m = 10.0;
auto result = p50 * m;

The result should be 5.0_m, and based on the conversions in play that requires a double value of 0.5;

None of this is to say that it shouldn't be different, that's just the background, but I've yet to find a solution to the consistency issue that didn't just move the problem and break something else. It's an active area of work but I'm not sure it can be solved without dimensioning dimensionless units (which could be done - but it's a can of worms especially when it comes to angles and physics)

@ts826848
Copy link
Contributor

ts826848 commented Jan 12, 2023

I wonder whether there actually can be a general scalable solution. I think the solution that has the least potential for confusion in this situation is to have custom functions to conversion to/from built-in types (for example, from_percentage_points, from_ratio, to_percentage_points, to_ratio for percent<>), but I worry that that wouldn't scale very well, as I don't think it would be compatible with the various UNIT_ADD macros. I think it'd also require some other changes, like a implicitly_convertible_from_scalar trait that is added to the scalar unit constructors/conversion operators. Still feels a bit hacky, unfortunately.

I'd guess similar concerns would also apply to the other types in concentration.h? Not sure about other dimensionless units like angles, but in theory those can be left alone for now.

@chiphogg
Copy link

Related issues:

I think the root problem is that the notion of the "value" of a quantity is dangerously ambiguous exactly when we have a dimensionless quantity with a magnitude other than 1. I discussed this a little bit in this comment: #275 (comment)

I think the upshot is that these units can't have implicit conversions, in either direction. When we initialize a percent quantity by assigning 1.0 to it, either 100% or 1% are perfectly reasonable answers. Asking "which one is right" is a recipe for confusion. Instead, save implicit conversions for the cases where an unambiguous correct answer exists.

@esppat
Copy link
Author

esppat commented Jan 13, 2023 via email

@chiphogg
Copy link

My initial issue was not to set a value of 1 to a percent should result in 1% or 100%, but assigning a double to a percent then getting back this double front the percent should give the original double value.

Sorry, I had a hard time understanding the point you were trying to get across. Was it...

I was going to respond, but I wanted to make sure I wasn't talking past you. 🙂

@esppat
Copy link
Author

esppat commented Jan 14, 2023 via email

@chiphogg
Copy link

My problem is the second sentence:
I don't care whether assigning 1.0 to percent_t gives 1% or 100%; I just care that it returns 1.0 when I assign it back to a double

In other words :

double d = 1.0;
percent_t p = d;
double d2 = p;

d2 should be equal to d ...

OK, thanks for clarifying!

For d2 to equal d, then we must choose one of those two conventions, even if you personally don't care which we choose. Let's game it out.

If we choose the "underlying value" convention (so that 1.0 is equivalent to 1%), then multiplying by percent_t{1.0} is the same as multiplying by 1.0. This is clearly wrong, so I think we can safely eliminate this option.

Therefore, we're left with the "scalar value" convention (so that 1.0 is equivalent to 100%). In order to make the round-trip consistent, we need to make the assignment operator semantically different from the copy constructor by a factor of 100:

percent_t p1 = 1.0; // 100%
percent_t p2{1.0};  // 1%

This seems completely unworkable because it violates deeply held intuitions about how C++ types should behave.

Thus, we've eliminated both options.

The only workable solution I've ever seen to this problem is to disable the implicit constructors and conversion operators for precisely these units: dimensionless units other than "the unit 1". Implicit conversions never make sense unless an operation is completely unambiguous. But for these units, the operation is ambiguous. Therefore, we can rule out supporting implicit conversions.

@esppat
Copy link
Author

esppat commented Feb 6, 2023 via email

ts826848 pushed a commit to ts826848/units that referenced this issue Mar 27, 2023
Implicit conversions to/from built-in types appear to be a potential
source of confusion for dimensionless units with a scaling factor (e.g.,
percent<>) [0]. Disallowing implicit conversions in those cases might be
a not-terrible way to eliminate this potential for confusion without
decreasing usability.

The idea for limiting the implicit conversion to unscaled values was
adapted from mpusz/units [1]. I'm not entirely confident this
implementation is the best approach, but it's at least not obviously
broken.

[0]: nholthaus#301

[1]: mpusz/mp-units#412 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants