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

Error comparing temperature units with offset #3134

Open
dvd101x opened this issue Jan 21, 2024 · 11 comments
Open

Error comparing temperature units with offset #3134

dvd101x opened this issue Jan 21, 2024 · 11 comments

Comments

@dvd101x
Copy link
Collaborator

dvd101x commented Jan 21, 2024

Hi,

In versiones newer than 11.4.1 the comparison of temperatures is incorrect between some temperature units when an offset is involved.

lowTemp = 9 degC;
highTemp = 10 degC;

lowTemp         < highTemp          # true
lowTemp to degF < highTemp          # false but should be true
lowTemp to K    < highTemp          # false but should be true
lowTemp to degR < highTemp          # false but should be true
lowTemp to K    < highTemp to degF  # false but should be true
lowTemp to degR < highTemp to degF  # false but should be true

This was discussed in #2518 but this might need to be a separate issue.

@josdejong
Copy link
Owner

josdejong commented Jan 24, 2024

Thanks for bringing up this bug.

I had a short look around, and this looks like a serious issue: there are quite some places where the unit.value is used without reckoning with an offset: compareUnits (internal function used by the relational functions compare, equal, unequal, smaller, smallerEq, larger, largerEq), addScalar, subtractScalar, sign, unaryMinus, not, isNegative, isPositive, isZero. All these functions directly use .valueType() and the .value property of the unit. Instead, we should create some utility method in Unit, like .absValue() that applies any offset.

Does that make sense @gwhitney ?

See also #2499, #2501

@gwhitney
Copy link
Collaborator

I mean it's all related to the contrast between absolute temperatures and differences between two temperatures and losing track of which a given temperature unit entity is supposed to be. All these things should be moved to a separate Unit class and solved there. I am at a bit of a loss of how to solve then while Unit is integrated in mathjs. I am in fact getting to be sorry I meddled in temperature units at all -- I fear I made things worse :-(

@josdejong
Copy link
Owner

Yes indeed. It is tricky stuff.

Do you think that implementing a utility method like .absValue() and use that everywhere in the code base instead of .value would solve this in a neat way?

@gwhitney
Copy link
Collaborator

Well if it were up to me I would have totally distinct units for temp differences and absolute temperatures. But thinking back to earlier issues/discussions, that didn't seem to be the consensus solution. But short of that, I don't understand what to do when asked to compare 200 degK and 300 degK -- if one is a temperature difference and the other a specific temperature, the comparison doesn't actually make sense as far as I can see...

@gwhitney
Copy link
Collaborator

If I recall, the suggestion was that F and C should only be used for specific temperatures and that taking a difference would always end up with R or K, where the contrast between temp differences and specific temps is less of an issue b/c of existence of absolute zero. (So arithmetic with degC would auto convert to degK, for example.) Then the current .value() should work just like the .absValue() you want -- that's how it was supposed to work originally, and you wouldn't need a new method. So if I had to try to fix this, I would read all the temperature unit issues, and if they all seemed consistent with that plan, I'd head that way.

@josdejong
Copy link
Owner

I don't remember all the details of earlier discussions either 😅.

Originally, the Unit.value property did contain the normalized value including any offset (i.e. temperatures in absolute Kelvin, not relative).

Let me try to summarize the different topics to see where we stand (the "desired behavior" is now under discussion, I filled in what makes most sense to me):

description example [email protected] and older [email protected] and newer desired behavior
value property math.unit('10degC').value 283.15 10 ???
add/subtract (A) 5 degC + 10 degC 288.15 degC 15 degC 15 degC
add/subtract (B) 5 degC + 10 K 15 degC 15 degC 15 degC
multiply/divide (A) 5 J/K * 2 K 10 J 10 J 10 J
multiply/divide (B) 5 J/degC * 2 degC 5 J 10 J 10 J
compare 10 degC == 283.15 K true false true

To fix the "value property" issue we can go two ways I think:

  1. keep the value without offset and introduce a new method like .valueWithOffset() to get the value with offset included. Adjust all functions that currently use .value to use .valueWithOffset() instead.
  2. change the value property back to include the offset, and adjust all operations like add/subtract/multiply/divide to work correctly.

Option (1) is quite straightforward, but I'm not sure how much work is involved in option (2). Any idea?

@gwhitney
Copy link
Collaborator

It's not possible to fill in the table without deciding what entities you want to allow to be temperature deltas and what ones are only allowed to be specific temperatures. If I am recalling, one proposal was that celsius and fahrenheit may only be used for specific temperatures, whereas we would allow Kelvin and Rankine for temp differences. In that case, the desired values column would become:

math.unit('10degC').value: 283.15
5 degC + 10 degC: Error: can not add specific temperatures
5 degC + 10 K: 15 degC
5 K + 10 degC: 15 degC
5 K + 10 K: 15K
10 degC - 5 degC: 5 K
10 degC - 5 degK: 5 degC
10 K - 5 degC: -268.15 K
10 K - 5 K: 5 K
5 J/K * 2 K: 10 J
5 J/degC: Error: can not divide by a specific temperature. Use K instead.
10 degC == 283.15 K: True

I think if you look back at other issues, they would work fine with this change. I'd be comfortable going in this direction if we have to fix this within mathjs, i.e. before things are farmed out to independent Unit class.

@dvd101x
Copy link
Collaborator Author

dvd101x commented Jan 25, 2024

I agree with the proposal Glen is mentioning and is consistent with other software I know. Where it's the user's responsibility to do temperature differences with Kelvin and Rankine.

NOTE: The CONVERT function will convert temperature differences but it does NOT convert temperatures from one scale to another. Use the CONVERTTEMP function for this purpose.

Use the CONVERT function to convert differences in temperature from one scale to another with DELTAF and DELTAC as unit strings.

Maybe there is a mathjs way of solving this problem but it seems complicated.

@josdejong
Copy link
Owner

It makes sense indeed to throw an error when trying to add two temperatures like 5 degC + 10 degC, and only allow adding a temperature in K. And the same with division.

Should we maybe also be more restrictive with subtraction, and not allow 10 K - 5 degC for example to prevent confusion? In your proposal this expression results in -268.15 K, but if I would reorder 10 K - 5 degC as -5 degC + 10 K, I would intuitively think that it would give the same result, but in that case I would expect 5 degC as result.

Thanks Glen, It would be great if you could work out a solution for this!

@gwhitney
Copy link
Collaborator

I see three ways to go with subtraction:

  1. Always prefer to interpret it as finding the temperature interval between two specific temperatures, whenever that interpretation is possible. That's the spirit in which 10K - 5degC is -268.15K.
  2. Throw an error when a particular use of - could be interpreted as either (1) or (3).
  3. Always prefer to interpret it as adding the negative of a temperature offset to a specific temperature, whenever that interpretation is possible. That's the spirit in which 10 degC - 5K is 5 deg C.

As implied by my values above, I prefer 1, because addition is naturally restricted to case (3) -- you could write 10 degC + (-5 K) -- so you get more well-defined behavior by pushing subtraction toward (1).

Note that in none of these interpretations would I advocate 10K - 5degC to be interpreted as moving -5degC by 10K, since degC should not be used for a temperature offset, so the interpretation of subtraction as applying the negation of a temperature offset is not available. So all that is left in this case is interpretation as the temperature distance between two specific temperatures, whichever of (1)-(3) above is adopted.

@josdejong
Copy link
Owner

Thanks for your explanation, your option (1) indeed makes sense, calculating it as a temperature difference. If that still results in confusion, we could look into option (2) and make it more restrictive, but no need for it now I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants