Conversions for temp (degress C<->F<->K) wrong about js-quantities HOT 11 CLOSED

I fixed this in my fork and sent a pull request.

from js-quantities.

I have to wonder why the code originally included degK and tempK as separate units with separate bases...

Since they were identical, I made them interchangeable, but was I wrong to do so?

Is one of them supposed to be measured as a kelvin-relative vector (1K == 1C == 5/9F), and the other as a kelvin-relative temperature (32F == 0C == 273.15K)?

My fix makes a check to see if the units are to be used in a ratio ("1mm/degC" or "1degC/min") to be calculated as a vector without base offset, or as a number of units ("1degC") to be calculated as a temperature with base offset. Should I have made it simpler by always calculating one with and the other without?

from js-quantities.

The code is based on ruby-units unit definitions.
Temperatures (tempX) are a very special case because there are relative and not absolute as other units (including degX).

Currently, js-quantities does not handle relative temperatures because they had no use for me.

If implemented, the code should handle them as below:

1degK == 1degC == 5/9degF // Differences
32tempF == 0tempC == 273.15tempK

Moreover,

• operations between quantities should handle automatic conversions
  tempX - tempX => degX
  tempX + degX => tempX
• operation other than difference between 2 tempX are forbidden:
  tempX (+|*|/) tempX => error

from js-quantities.

This is more complicated than I expected. My code works for simple cases, but not complicated math. I'd like to treat them both as vector and scalar, where rhs is always vector and if the lhs is ever multiplied or divided by a non-unitless qty it is first turned into vector.

But that does not allow things like: 2tempC - 2tempF == 18.667tempK

To me the above seems like wrong behavior anyway, but maybe that is why they separated them, so that you can get what seems correct to me by 2tempC - 2degF, and people who expect the other behavior can still get it by doing the above.

from js-quantities.

I added some unit tests for more complicated math and made changes to the code so that they pass.

They do not follow exactly what you put up above (ie: "degX" is still synonymous with "tempX"). All math operations are allowed, but I think they are done in a sane way with unit tests to prove that they do what is expected. I start by not converting temperatures to base units in math operations.

To start out, I'll mention the possible issue in case it is a deal breaker (this probably deserves a short blurb in the readme also):
...since:
"2degC/s".to("degF/s") == "(18/5) degF/s" (converted as vector, as in this case of exaggerated global warming)
...then:
"2degC/s".to("degF/s").mul("s") == "(18/5) degF" (which could be converted to degC as a temperature that is not 2)

...Now, the general behavior as I intended it (and believe I achieved)...

There are some rounding issues, but there always were and these values are very close (hence the unit tests using toBeCloseTo rather than toBe):

"0degC".to("degF") == "32degF" ("to" function converts temperature values, not degrees)
"0degC".add("2degC") == "2 degC"
"0degC".add("2degF") == "2_5/9 degC"
"0degC".sub("2degF") == "-2_5/9 degC"

"2degC".mul("2degF") == "4 degC*degF" (you must then divide by degF or degC to get back to a sane unit)
"2degC/degF".mul("2degF") == "4 degC"

"4degC*degF".div("2degF") == "2 degC"
"4degC".div("degF") == "4degC/degF"
"4degC".div("2degC") == "2" (unitless quantity)

If this looks sane to you, or close to sane, feel free to add any unit tests (broken or passing) that make sense and I will see if I can make any broken ones work.

If this does not look sane to you, and you can think of unit tests that do, then please still feel free to push them (as long as they cover deg/temp conversion and math, throwing errors or returning results) and I'll see what I can do about them.

from js-quantities.

That looks fine. I can't check right now because I'm on vacation without
any computer (I'm answering with my smartphone) but I'll try as soon as
possible.
Le 3 juil. 2013 01:37, "Shadow Man" [email protected] a écrit :

I added some unit tests for more complicated math and made changes to the
code so that they pass.

They do not follow exactly what you put up above (ie: "degX" is still
synonymous with "tempX"). All math operations are allowed, but I think they
are done in a sane way with unit tests to prove that they do what is
expected.

To start out, I'll mention the possible issue in case it is a deal breaker
(this probably deserves a short blurb in the readme also):
...since:
"2degC/s".to("degF/s") == "(18/5) degF/s" (converted as vector, as in this
case of exaggerated global warming)
...then:
"2degC/s".to("degF/s").mul("s") == "(18/5) degF" (which could be converted
to degC as a temperature that is not 2)

...Now, the general behavior as I intended it (and believe I achieved)...

There are some rounding issues, but there always were and these values are
very close (hence the unit tests using toBeCloseTo rather than toBe):

"0degC".to("degF") == "32degF" ("to" function converts temperature values,
not degrees)
"0degC".add("2degC") == "2 degC"
"0degC".add("2degF") == "2_5/9 degC"
"0degC".sub("2degF") == "-2_5/9 degC"

"2degC".mul("2degF") == "4 degC*degF" (you must then divide by degF or
degC to get back to a sane unit)
"2degC/degF".mul("2degF") == "1 degC"

"4degC*degF".div("2degF") == "2 degC"
"4degC".div("degF") == "4degC/degF"
"4degC".div("2degC") == "2" (unitless quantity)

If this looks sane to you, or close to sane, feel free to add any unit
tests (broken or passing) that make sense and I will see if I can make any
broken ones work.

If this does not look sane to you, and you can think of unit tests that
do, then please still feel free to push them (as long as they cover
deg/temp conversion and math, throwing errors or returning results) and
I'll see what I can do about them.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1#issuecomment-20386572
.

from js-quantities.

...in case you don't like my changes...

FYI - I've looked at the ruby code at https://github.com/olbrich/ruby-units and apparently they treat temp and deg the same as far as being without base offset, but they do specific checks in the add/sub/div/mul/to methods for temp in the signature. The math methods throw exceptions if a temp is on the wrong side of the equation and the to method handles the base offset when converting to other temps.

That could be implemented in your javascript code in those (and related) methods without adding the base offset field to UNIT definitions. Although there will still need to be cleanup to keep temp/deg objects from ever being converted toBase (for instance inside the mul/div functions, which are legal for deg units).

If you care to look for yourself, see lib/ruby_units/unit.rb definitions for usage of 'is_temperature?'.

from js-quantities.

js-quantities was based on a (very) older version of ruby-units whose
temperatures implementation was incomplete as far I remember. But I had no
use of them, that's why I didn't fix it in the past.

I will look the current implementation in ruby-units.
Le 11 juil. 2013 21:19, "Shadow Man" [email protected] a écrit :

FYI - I've looked at the ruby code at
https://github.com/olbrich/ruby-units and apparently they treat temp and
deg the same as far as being without base offset, but they do specific
checks in the add/sub/div/mul/to methods for temp in the signature. The
math methods throw exceptions if a temp is on the wrong side of the
equation and the to method handles the base offset when converting to other
temps.

That could be implemented in your javascript code in those (and related)
methods without adding the base offset field to UNIT definitions. Although
there will still need to be cleanup to keep temp/deg objects from ever
being converted toBase (for instance inside the mul/div functions, which
are legal for deg units).

If you care to look for yourself, see lib/ruby_units/unit.rb definitions
for usage of 'is_temperature?'.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/1#issuecomment-20835069
.

from js-quantities.

Just to see what it would look like (and because I don't like the look of my toBaseUnits and fromBaseScalar functions), I made a branch on my repo for a ruby-units variant.

The unit tests were also modified to correspond with what I believe is the ruby-units behavior (I've never run anything ruby, but I think I understand what the code is saying).

It should be finished, although it is not merged into master as I'm not certain which one is better.

from js-quantities.

Ok, I've merged the ruby-units variant into master. Since this is supposed to be a port of ruby-units, that behavior is what should be used.

from js-quantities.

Closing now since @rage-shadowman's pull request #2 is merged.

from js-quantities.