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

Separate absolute and relative temperature #176

Closed
wants to merge 5 commits into from

Conversation

cstjean
Copy link
Contributor

@cstjean cstjean commented Oct 19, 2018

Following the discussion in #137, I separated absolute and relative temperature units, similar to @burnpanck's suggestion. It works well so far. Notable new test cases:

            @test uconvert(abs°F, 0abs°C) == 32abs°F
            @test uconvert(°F, 0°C) == 0°F
            @test 1.0abs°C + 45°F == 26.0abs°C
            @test 1abs°C - 1K == 0abs°C
            @test 40abs°C - 95abs°F == 5°C
            @test_throws DimensionError 1abs°C + 1abs°F

This PR is a proposal for discussion. If the general approach is acceptable, then we might find a less ugly name than 35abs°C. And of course: documentation.

Regarding binary arithmetic on absolute temperatures, we can either disallow promotion of two absolute temperatures in general, and explicitly allow some operations (-, ==, ...?) or allow all operations by default and disallow some (+, *, /, ...?) by throwing an exception. I did the latter in this PR, but I also tested the former and it works too.

Fix #137 and #72

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #176 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #176      +/-   ##
=========================================
- Coverage   97.53%   97.4%   -0.14%     
=========================================
  Files          15      15              
  Lines         691     694       +3     
=========================================
+ Hits          674     676       +2     
- Misses         17      18       +1
Impacted Files Coverage Δ
src/temperature.jl 94.11% <100%> (-5.89%) ⬇️
src/pkgdefaults.jl 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e5c2a4...a16f0b7. Read the comment docs.

@cstjean
Copy link
Contributor Author

cstjean commented Oct 19, 2018

Here's one issue:

julia> mean([1abs°C, 2abs°C])
ERROR: DimensionError: 1 abs°C and 2 abs°C are not dimensionally compatible.
Stacktrace:
 [1] +(::Quantity{Int64,Unitful.Dimensions{(Unitful.Dimension{:AbsTemperature}(1.

We can fix it manually, but it's a bit worrying.

@RuiRojo
Copy link

RuiRojo commented Oct 19, 2018

(A nobody giving his opinion)

I wouldn't pay much attention to the mean issue. It just happens that the general implementation of mean assumes you can add the stuff, while that's not a requirement for a mean to make sense. mean_v2(x) = mean(x .- x[1]) + x[1].

I understand the appeal to make the new weirdos in town (units with offsets) bear the prefix. But I think that it would introduce less silent mistakes to make the traditional names (°C, °F, K) default to the absolute ones which IIUC would error more often when misused (you can't add them, multiply, etc). As is,uconvert(°F, 0°C) returning 0°F smells like trouble. Maybe it's even worth it to have a short prefix for both relative and absolutes.

@ajkeller34
Copy link
Collaborator

@RuiRojo Nobody's a nobody, thanks for your interest! Not sure what to think yet regarding the unit names.

@cstjean Thanks for the PR. I think I agree with the general premise (still open to further discussion of unit names) but I don't think there should be a new dimension defined. The dimension of temperature ought to be the same regardless of absolute / relative scales. I imagine you were trying for a minimally invasive mechanism to identify absolute / relative scale from the type, but rather than compromise on dimensional correctness I think we could generalize the Units types to permit affine transformations. Perhaps an extra type parameter that could either be nothing (no affine transformation) or a value ("absolute zero" on the scale).

@cstjean
Copy link
Contributor Author

cstjean commented Oct 25, 2018

Thank you for the comments. I agree that a general solution would be better.

The dimension of temperature ought to be the same regardless of absolute / relative scales.

Conceptually, I agree, but pragmatically, I can define functions with dimensions in the signature, like is_boiling(::Temperature) = t < 100°C. That function should not accept relative temperature, but I can't specify that if absolute and relative temperatures share the same dimension.

I think we could generalize the Units types to permit affine transformations. Perhaps an extra type parameter that could either be nothing (no affine transformation) or a value ("absolute zero" on the scale).

Sounds good.

The current PR is good enough for our purposes, so I don't know when/if I'll have time to push this further. If anyone wants to take a shot at this, please do.

@burnpanck
Copy link

This is exciting! I'm not even a Julia user yet, but it's great to see how the language allows library writers to offer tools that do the right thing™, without sacrificing expressiveness. And of course, kudos to you guys! Now if only arrays would start at zero...

@ajkeller34
Copy link
Collaborator

Closed in favor of #177, discussion can continue there.

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.

None yet

4 participants