Skip to content

Fix arithmetic for Temperature #219

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

Merged
merged 7 commits into from
Jan 28, 2017
Merged

Fix arithmetic for Temperature #219

merged 7 commits into from
Jan 28, 2017

Conversation

angularsen
Copy link
Owner

@angularsen angularsen commented Jan 22, 2017

Fixes #218

  • 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

@angularsen
Copy link
Owner Author

@SirRufo Can you look over and comment?

I made a stab at this, but having problem with arithmetic doing things like Temperature.FromDegreesCelsius(10) * 2 == Temperature.FromDegreesCelsius(20).

See TemperatureTests.cs for commented tests. I think these should be doable, but my brain gave up and wants to sleep :-)

@angularsen
Copy link
Owner Author

On a side note, the whole inherit units stuff was a side track, as I wound up not using it when I realized the conversions were different. I am keeping it, as it's been a topic of discussion before. Might come in handy later.

@SirRufo
Copy link

SirRufo commented Jan 23, 2017

We cannot add two temperatures or multiply a temperature by a n (which is just adding for n times), because it does not make sense at all.

If we measure two cubes with each have a side length of 1 m, putting them together will indeed double the volume (2 * 1 cbm = 2 cbm), but the temperature of both (20°C measured for each) will not double - it will remain the same at 20°C. If the temperature is different (10°C and 20°C) then putting both together we will have a total temperature of 15°C (assuming both are of the same homogene material and ideal conditions).

The whole temperature arithmetic has to be done with the delta values.

BTW: I would also remove the - operator from temperature type.

{
public partial struct Temperature
{
public static Temperature operator -(Temperature right)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this

@angularsen
Copy link
Owner Author

Thanks, yes I think you are right. As much as 2 * 20 degrees Celsius should be 40 degrees Celsius, this must be multiplied by a temperature delta of 2 degrees Celsius. After a good night's sleep I finally realized that the different temperature scales have different zero points in addition to different scaling factors, so you can't simply multiply by two, but you can multiply or divide by some amount within each scale - the TemperatureDelta.

I'll remove the number arithmetic and only keep the Delta arithmetic with regards to Temperature, including the negate - operator. Same thing there.

So, the next thing that comes to mind. Are there other unit classes beyond Temperature that also has this problem of arithmetic with numbers not making sense?
Logarithmic units come to mind, but we already have a special case for these.

@angularsen angularsen force-pushed the add-temperaturedelta branch from ef94181 to 8e1d45f Compare January 24, 2017 22:35
@angularsen
Copy link
Owner Author

angularsen commented Jan 24, 2017

@SirRufo Please review again. I found that * and / operators would not work with TemperatureDelta. I realized that you need to perform those operations within the context of a particular unit, due to different zero points of different units.

Temperature.FromDegreesCelsius(20) * TemperatureDelta.FromDegreesCelsiusDelta(3) // NOT 60 degrees C..
Temperature.FromDegreesCelsius(20).Multiply(3, TemperatureUnit.DegreeCelsius) // 60 degrees C

Am I mistaken, or is this the path we must take for multiplication and division?

I've added some more tests now and all arithmetic seem to work as I expect now. Fahenheit and Celsius have different scales and different zero points, so they should cover most cases.

By the way, I did not mean to hijack your PR, but I went down a rabbit hole and have yet to climb out :-)
I invited you to the gitter chat in case we are online at the same time and can wrap this up more quickly.

@SirRufo
Copy link

SirRufo commented Jan 25, 2017

I only have a look at the units ...

x°C * y°C = x*y°C2.

Sure we need this?

That are the ones I see at first:

double operator /( TemperatureDelta a, TemperatureDelta b )
TemperatureDelta operator /( TemperatureDelta a, double b )
TemperatureDelta operator *( TemperatureDelta a, double b )
TemperatureDelta operator *( double a, TemperatureDelta b )

Useful units (which are new to your lib) dealing with TemperatureDelta would be:

  • heat capacity, with unit [J K−1]
  • specific heat capacity, with unit [J kg−1 K−1]
HeatCapacity = Energy / TemperatureDelta
SpecificHeatCapacity = HeatCapacity / Mass
SpecificEnergy = Energy / TemperatureDelta
SpecificHeatCapacity = SpecificEnergy / Mass

but that is another story

@angularsen
Copy link
Owner Author

x°C * y°C = x*y°C

Where did you see this? I thought I removed all Temp * Temp calculations and tests.
What I did add, was Temperature Multiply(double factor, TemperatureUnit unit) and Temperature Divide(double divisor, TemperatureUnit unit), as in my previous example.

The TemperatureDelta operator overloads you mention should also be implemented, if you were asking to implement them?

Good point about the new units HeatCapacity and SpecificHeatCapacity, you are welcome to add these units and operator overloads in a PR.

@angularsen
Copy link
Owner Author

angularsen commented Jan 28, 2017

So from what I can tell, this PR is now ready to merge.

  • Default arithmetic with Temperature removed, they were plain wrong
  • Custom arithmetic for Temperature and TemperatureDelta is added, also a custom Multiply() and Divide() method since this is easy to get wrong for the consumers of the lib.
  • TemperatureDelta added with default arithmetic, which should cover the examples you gave above

Defaults to true if not set (backwards compatible).
Set to false to not generate default arithmetic for a unit class,
such as Temperature that must have custom arithmetic.
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
Inherits units from Temperature.
Use TemperatureDelta for + and - operators.
Cannot use TemperatureDelta with * and / operators, since different temperature
units have different zero points, so one must first convert to a unit of
choice, then perform the arithmetic, then convert back to Temperature.
@angularsen angularsen force-pushed the add-temperaturedelta branch from 8e1d45f to 04ced50 Compare January 28, 2017 14:58
@angularsen angularsen changed the title Add TemperatureDelta (WIP) Fix arithmetic for Temperature Jan 28, 2017
@angularsen angularsen merged commit 7a455f0 into master Jan 28, 2017
@angularsen angularsen deleted the add-temperaturedelta branch January 28, 2017 15:03
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

Successfully merging this pull request may close these issues.

2 participants