search

nholthaus / units

a compile-time, header-only, dimensional analysis and unit conversion library built on c++14 with no dependencies.
http://nholthaus.github.io/units/
MIT License
964 stars 137 forks source link

percent_t FROM double and TO double are different #301

Open esppat opened 2 years ago

esppat commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/nholthaus/units/issues/275#issuecomment-1275015653

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 commented 1 year ago

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.

13 janv. 2023 05:18:03 Chip Hogg @.***>:

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)[https://github.com/nholthaus/units/issues/275#issuecomment-1275015653]

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.

— Reply to this email directly, view it on GitHub[https://github.com/nholthaus/units/issues/301#issuecomment-1381297235], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABPAFIANARKSF6EPH7OS273WSDJM7ANCNFSM6AAAAAASLFR3JA]. You are receiving this because you authored the thread.[Image de pistage][https://github.com/notifications/beacon/ABPAFIGBATE3TVIIDXWSAULWSDJM7A5CNFSM6AAAAAASLFR3JCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSSKTWFG.gif]

chiphogg commented 1 year ago

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. :slightly_smiling_face:

esppat commented 1 year ago

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 ...

13 janv. 2023 13:32:29 Chip Hogg @.***>:

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. 🙂

— Reply to this email directly, view it on GitHub[https://github.com/nholthaus/units/issues/301#issuecomment-1381789425], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ABPAFIBPBW4L3GQZPOK2UDTWSFDLDANCNFSM6AAAAAASLFR3JA]. You are receiving this because you authored the thread.[Image de pistage][https://github.com/notifications/beacon/ABPAFIB77ICTVGU6LJLK3P3WSFDLDA5CNFSM6AAAAAASLFR3JCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTSSLRXPC.gif]

chiphogg commented 1 year ago

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 commented 1 year ago

I do not see thinks like that. Back to my example:

double d = 1.0; percent_t p = d; double d2 = p; Could be modified a bit to be clearer:

double d, d2; percent_t p;

d = 10.0; p = d; d2 = p;

Here, d and d2 should be equal: 10.0

I do not mix affectation with initialization, which are different thinks for me. But I may be wrong ... Look:

percent_t p1 = 1.0; percent_t p2{1.0};

I could accept (and may be understand) that here, p1 and p2 are of different values; But affectation IN and OUT should be consistent ...

But, I have to say, whatever the future decision about this detail is (change something or not), I already found a workaround for my own usage and I am fine with the library as is.

(sorry for English mistakes, I am a poor old Frenchie ...) Thanks a lot for your attention Patrice

Le sam. 14 janv. 2023 à 22:59, Chip Hogg @.***> a écrit :

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.

— Reply to this email directly, view it on GitHub https://github.com/nholthaus/units/issues/301#issuecomment-1382934652, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPAFIHS3BAJKJ5BFBU5I6DWSMOVPANCNFSM6AAAAAASLFR3JA . You are receiving this because you authored the thread.Message ID: @.***>