Skip to content

Temperature arithmetic is wrong #218

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

Closed
SirRufo opened this issue Jan 21, 2017 · 13 comments
Closed

Temperature arithmetic is wrong #218

SirRufo opened this issue Jan 21, 2017 · 13 comments

Comments

@SirRufo
Copy link

SirRufo commented Jan 21, 2017

According to https://en.wikipedia.org/wiki/Celsius#Temperatures_and_intervals

What is often confusing about the Celsius measurement is that it follows an interval system but not a ratio system; that it follows a relative scale not an absolute scale. This is put simply by illustrating that while 10 °C and 20 °C have the same interval difference as 20 °C and 30 °C the temperature 20 °C is not twice the air heat energy as 10 °C. As this example shows, degrees Celsius is a useful interval measurement but does not possess the characteristics of ratio measures like weight or distance.

and further more at https://de.wikipedia.org/wiki/Grad_Celsius#Temperaturdifferenz (german)

delta t = 2°C - 1°C  is equivalent to delta T = 2K - 1K
delta t = 1K (= 1°C) is equivalent to delta T = 1K (not equal -272.15°C)

And that will fail here

var delta_t = 2.DegreesCelsius( ) - 1.DegreesCelsius( );
var delta_T = 2.Kelvins( ) - 1.Kelvins( );

System.Diagnostics.Debug.Assert( delta_t == delta_T );
System.Diagnostics.Debug.Assert( delta_t.Kelvins == delta_T.Kelvins );
System.Diagnostics.Debug.Assert( delta_t.DegreesCelsius == delta_T.DegreesCelsius );
System.Diagnostics.Debug.Assert( delta_t.Kelvins == 1 );
// next will fail because delta_t.DegreeCelsius is -272.15
System.Diagnostics.Debug.Assert( delta_t.DegreesCelsius == 1 );
@angularsen
Copy link
Owner

I follow what you say. I think in this case, the arithmetic of subtracting one temperature from the other cannot be thought of as an interval, but rather a new temperature.

So in your example, subtracting 2K - 1K is not an interval of 1K, but a new temperature of 1K.
I don't think the implementation is wrong, at least the way I think about it, but rather what you expect when subtracting two temperatures.
To achieve this, I propose a new method TemperatureInterval GetInterval(Temperature other) and a new type TemperatureInterval having the same units, but a different semantics. You can then maybe add/subtract temperatures from temperature intervals. I think a .NET analogy would be TimeSpan vs DateTime. You can combine the two types and perform arithmetic between them, but they mean different things.

@SirRufo
Copy link
Author

SirRufo commented Jan 21, 2017

The example shows what happens if you subtract a measured temperature from another measured temperture: You get a delta and not a new temperature.

// calculates ( -271.15°C / 2K ) - ( -272.15°C / 1K )
// will evaluate to 1°C / 1K
TemperatureDelta t = TemperatureMeasure.FromKelvin( 2 ) - TemperatureMeasure.FromKelvin( 1 );

The TemperatureDelta can be handled as any other dimension (adding delta, multiply by value) and can be added to a TemperatureMeasure.

// will evaluate to 2°C / 275.15K
TemperatureMeasure t = TemperatureMeasure.FromCelsius( 1 ) 
  + TemperatureDelta.FromCelsius( 1 );

// will evaluate to 3°C / 276.15K
TemperatureMeasure t = TemperatureMeasure.FromCelsius( 1 ) 
  + 2 * TemperatureDelta.FromCelsius( 1 );

with the current implementation I will get

// will evaluate to 275.15°C / 548.3K
Temperature t = 1.DegreeCelsius() + 1.DegreeCelsius();

@angularsen
Copy link
Owner

I apologize, I finally got my head the right way around this. You are absolutely correct.

In the analog of time, Temperature is DateTime and when subtracting two DateTimes you get TimeSpan, which would be our TemperatureDelta. You can add/subtract/multiply/divide a Temperature with a TemperatureDelta. You cannot do DateTime + DateTime, in the same way Temperature + Temperature don't make sense. I suppose the confusing part was that we talk about degrees Celsius as both a measurement and an interval/delta, while in time we have different units for this.

So back to the implementation proposal:

  • Remove current generated arithmetic for Temperature (breaking change)
  • Add new unit TemperatureDelta with same units as Temperature, ideally our code generator supported some inheritance or aliasing, so we could define TemperatureDelta to be the same as Temperature, except different naming.
  • Add custom arithmetic between Temperature and TemperatureDelta

@angularsen
Copy link
Owner

Would you be interested in doing a pull request on this?

@SirRufo
Copy link
Author

SirRufo commented Jan 22, 2017

Temperature is used in speech and writing for both meanings Delta and Absolute/Measured and we had to read between the lines what is really mentioned here.

I am thinking about two new types named TemperatureDelta and TemperatureMeasured (or TemperatureAbsolute) to avoid conflicts with the current implementation of Temperature type.

Extensions can be done for both:

TemperatureDelta td = 35.DegreeCelsius();
TemperatureMeasured tm = 35.MeasuredDegreeCelsius();
// or
TemperatureAbsolute ta = 35.AbsoluteDegreeCelsius();

I am not quite sure what naming convention we should follow here and what would be the one most people will know at first sight what is going on and what does it mean.

Once we are both satisfied with the kids name I would like to offer a pull request to you :o)

@angularsen
Copy link
Owner

angularsen commented Jan 22, 2017

I am leaning towards keeping Temperature, to mean a measured temperature. That was always what it was meant to be. The only confusing part is that it currently has arithmetic that does not make sense, such as adding two temperatures and that subtracting two temperatures returns a new temperature instead of a delta.

It would be a breaking change, but on the other hand, the behavior/semantics are simply wrong, so I would be fine doing this change without bumping the major version of the lib.

I am currently looking into extending the generator scripts to support a InheritsUnitsFrom: [ "Temperature" ] property, with array of class names to inherit units from, so TemperatureDelta can copy the unit definitions of Temperature.
Then I will look into adding a property to omit the generated arithmetic, since that needs to be done manually in custom code.

@SirRufo
Copy link
Author

SirRufo commented Jan 22, 2017

Well thats fine for me.

How would the naming convention look like for the number extensions (which I like very much). Somehow like:

Temperature t = 34.DegreeCelsius();
TemperatureDelta td = 10.DeltaDegreeCelsius();

Temperature newTemperature = 34.DegreeCelsius() - 10.DeltaDegreeCelsius();

@angularsen
Copy link
Owner

Hm, I think the plural form DeltaDegreesCelsius() rings best in my ear? The current form is plural DegreesCelsius().

@SirRufo
Copy link
Author

SirRufo commented Jan 22, 2017

My german ears are more accurate on german language - so we best follow your ears in this case ;o)

@SirRufo
Copy link
Author

SirRufo commented Jan 22, 2017

Another possible option would be DegreesCelsiusDelta which would maybe useful with IntelliSense as a reminder. Typing DegreesCelsius will show you both options to mark it as a temperature or a delta.

Well typing Delta will show you all options related to delta temperatures which is also useful.

Do you feel we need both or would be Delta... enough?

@angularsen
Copy link
Owner

I was thinking the same thing, sure, DegreesCelsiusDelta is fine by me.
I would not add both. At least the ReSharper intellisense would give you all the hits when typing Delta, not sure about the Visual Studio intellisense.

@angularsen
Copy link
Owner

I am making progress on the InheritsUnitsFrom property, got a working prototype here now. Will push later today.

@angularsen
Copy link
Owner

Also a property for GenerateArithmetic: bool, which defaults to true.

angularsen added a commit that referenced this issue Jan 22, 2017
See discussion in related issue. The semantics of the current Temperature
arithmetic is plain wrong, such as adding 1 + 1 degrees Celsius is not
2 degrees Celsius, but rather +275.15 degrees Celsius.

The problem is how we in speech mixi temperature delta/interval and temperature
measurements, in contrast to time (DateTime vs TimeSpan, with different units).

Refs #218
#218
angularsen added a commit that referenced this issue Jan 24, 2017
See discussion in related issue. The semantics of the current Temperature
arithmetic is plain wrong, such as adding 1 + 1 degrees Celsius is not
2 degrees Celsius, but rather +275.15 degrees Celsius.

The problem is how we in speech mixi temperature delta/interval and temperature
measurements, in contrast to time (DateTime vs TimeSpan, with different units).

Refs #218
#218
angularsen added a commit that referenced this issue Jan 28, 2017
See discussion in related issue. The semantics of the current Temperature
arithmetic is plain wrong, such as adding 1 + 1 degrees Celsius is not
2 degrees Celsius, but rather +275.15 degrees Celsius.

The problem is how we in speech mixi temperature delta/interval and temperature
measurements, in contrast to time (DateTime vs TimeSpan, with different units).

Refs #218
#218
angularsen added a commit that referenced this issue Jan 28, 2017
Fix arithmetic for Temperature

 Default arithmetic with Temperature removed, they were plain wrong
 Custom arithmetic for Temperature and TemperatureDelta is added
 Add Temperature.Multiply() and .Divide(), easy to get wrong
 TemperatureDelta added with default arithmetic, which should cover the examples you gave above

Fixes #218
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

2 participants