add rankine temperature scale #161
Conversation
| * In consideration of these more unique scale conversions, two conversion types are supported: Degrees and Scale. | ||
| * | ||
| * Scale based conversions DO adjust for the zero offset. | ||
| * Thus 5 degrees C is the same as 41 degrees F on the thermometer. | ||
| * Thus 5 degrees C is the same as 41 degrees F on the thermometer.s |
There was a problem hiding this comment.
What's with the s at the end?
There was a problem hiding this comment.
That was a typo.
| @@ -47,32 +52,39 @@ class TemperatureSpec extends FlatSpec with Matchers { | |||
| x.toKelvinScale should be(0) | |||
| x.toFahrenheitScale should be(-459.67) | |||
| x.toCelsiusScale should be(-273.15) | |||
| x.toScale(Rankine) should be(0) | |||
There was a problem hiding this comment.
Why is this toScale unlike the other methods like toKelvinScale, etc. Should it be called toRankineScale instead?
There was a problem hiding this comment.
toKelvinScale, toCelsiusScale, and toFahrenheitScale are public convenience methods defined on the squants.thermal.Temperature class (see https://github.com/typelevel/squants/blob/master/shared/src/main/scala/squants/thermal/Temperature.scala#L112). The general form is Temperature.toScale(unit: TemperatureScale).
It makes sense to have these convenience methods for Kelvin, Celsius, and Fahrenheit, since they're so common. I chose not to defined a convenience method for Rankine since it's rarer. But if people feel strongly about it, I can add it.
There was a problem hiding this comment.
I don't have a strong opinion either way, it may look better to have it for consistency sake
| @@ -106,52 +121,63 @@ class TemperatureSpec extends FlatSpec with Matchers { | |||
| x.inFahrenheit should be(Fahrenheit(32)) | |||
| x.in(Kelvin) should be(Kelvin(273.15)) | |||
| x.in(Fahrenheit) should be(Fahrenheit(32)) | |||
| (x.in(Rankine) - Rankine(491.67)).value < 0.000000000001 should be(right = true) | |||
There was a problem hiding this comment.
I wonder if this is a good place to use the =~ operator with a given tolerance
There was a problem hiding this comment.
Probably. I adopted that style for consistency with the other tests in TemperatureSpec.
I'm not familiar with the =~ operator. Using +-, it would the line would look like this:
x.in(Rankine).value should be(491.67 +- 0.000000000001) which is less verbose than the current form, but doesn't test if the output is boxed in Rankine (it only tests the value conversion). I'm open to suggestions on how best to restructure this test, and we should probably restructure the tests in the Fahrenheit boxing tests in that file too (see https://github.com/typelevel/squants/blob/master/shared/src/test/scala/squants/thermal/TemperatureSpec.scala#L97 for example).
Overall, there's a lot of boilerplate and repeated code in the thermal package. The 9:5 scaling factor between Celsius and Fahrenheit is repeated multiple times, as are several other magic values. At some point, we should think about how to restructure the internals to reduce duplication and boilerplate.
|
|
||
| val c = Celsius(0) | ||
| c.toString(Kelvin) should be("273.15°K") | ||
| c.toString(Fahrenheit) should be("32.0°F") | ||
| c.toString(Celsius) should be("0.0°C") | ||
| c.toString(Rankine) should be("491.66999999999996°R") |
There was a problem hiding this comment.
Are the other scales truncated to 2 decimals?
There was a problem hiding this comment.
No, but the other examples convert cleanly to two decimals. 0C converts to 491.67R, but b/c of floating point arithmetic I had to include the full version. Since it's a string, I couldn't use tolerance.
|
PR updated based on feedback. |
|
I'll add Rankine as a supported Temperature Scale on https://github.com/typelevel/squants/wiki/Quantity-and-Unit-of-Measure-Reference and make a note of it in the 1.1.0-SNAPSHOT section on https://github.com/typelevel/squants/wiki/Release-History I'll do that after the PR is accepted. I'd also like some feedback on the ScalaDoc changes in Temperature.scala https://github.com/typelevel/squants/pull/161/files#diff-1baa53d268773fb89009e3cbf0d8d342R36 |
| @@ -33,13 +33,16 @@ import scala.util.{ Failure, Success, Try } | |||
| * Of course, these scales set their respective zero values well above absolute zero. | |||
| * This is done to provide a granular and reasonably sized ranges of values for dealing with everyday temperatures. | |||
| * | |||
| * This library supports another absolute scale, the Rankine scale. Rankine sets its zero at absolute zero, | |||
|
@derekmorr Is this PR ready to merge? |
|
I think so. |
|
Thanks for your PR! |
This closes issue #160 .