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

[RFC] Prototyping better temperature handling. #177

Merged
merged 9 commits into from Nov 1, 2018
Merged

Conversation

ajkeller34
Copy link
Collaborator

@ajkeller34 ajkeller34 commented Oct 26, 2018

Alternate approach to #176, will merge in a day if no further objections.

To-do:

  • bikeshed symbol names (is abs°C tolerable?)
    No need for new symbol names. Declare °C, °F are relative scales and shouldn't be used to represent temperature differences (that's what K and Ra are for)
  • bikeshed how to display quantities that affine transform under unit conversion (e.g. what we've been calling absolute temperatures). Right now they just show (affine) after the unit symbols, which is ugly.
    No need for different display.
  • restricted nonsensical operations for affine quantities (i.e. absolute temperatures): *, /, +, ^, inv, sqrt, cbrt, zero (could be allowed if different type could be returned), one, oneunit.
  • fix operations between absolute, relative temperatures
  • make things sensible when ContextUnits, FixedUnits, etc. are used
  • write documentation
  • make a macro for defining affine units more easily

@ajkeller34 ajkeller34 changed the title Prototyping better temperature handling. [WIP] Prototyping better temperature handling. Oct 26, 2018
@codecov
Copy link

codecov bot commented Oct 26, 2018

Codecov Report

Merging #177 into master will decrease coverage by 0.65%.
The diff coverage is 76.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #177      +/-   ##
==========================================
- Coverage   74.77%   74.12%   -0.66%     
==========================================
  Files          15       14       -1     
  Lines         904      939      +35     
==========================================
+ Hits          676      696      +20     
- Misses        228      243      +15
Impacted Files Coverage Δ
src/pkgdefaults.jl 2.7% <ø> (-4.99%) ⬇️
src/Unitful.jl 100% <ø> (ø) ⬆️
src/promotion.jl 25% <0%> (-4.17%) ⬇️
src/types.jl 81.81% <100%> (+1.81%) ⬆️
src/display.jl 65.38% <33.33%> (-5.45%) ⬇️
src/utils.jl 80% <50%> (-2.61%) ⬇️
src/user.jl 85.12% <80%> (-0.47%) ⬇️
src/conversion.jl 81.48% <81.25%> (-0.57%) ⬇️
src/quantities.jl 87.73% <81.25%> (-0.87%) ⬇️
src/units.jl 78.99% <90.9%> (+0.61%) ⬆️

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 0f52452...f95456c. Read the comment docs.

@coveralls
Copy link

coveralls commented Oct 28, 2018

Coverage Status

Coverage decreased (-0.7%) to 73.987% when pulling f95456c on temperature into 0f52452 on master.

@dnadlinger
Copy link
Contributor

dnadlinger commented Oct 28, 2018

Thanks! I've only glanced over the general affine quantity implementation, but a few quick comments regarding the temperature-specific bits:

  • The names absolute/relative are potentially confusing; in physics, "absolute temperature" is often used to mean thermodynamic/non-affine/… temperature scales like Kelvin: https://en.wikipedia.org/wiki/Absolute_temperature

  • I'd argue °C should become what is now abs°C, and the same for °F. Leaving them as they are seems dangerous, as they guide users unfamiliar with this issue to write invalid code by default. (There is no need to come up with names for the associated "relative temperature" scales, as they already exist: Kelvin for degrees Celcius, and Rankine for degrees Fahrenheit.)

@ajkeller34
Copy link
Collaborator Author

Ha, I was borrowing the terminology that #176 used and not thinking. You're absolutely right about that, though.

…suggestions.

- Celsius, Fahrenheit are always affine units now, and do not represent temperature differences.
@ajkeller34
Copy link
Collaborator Author

Alright, now °C and °F are affine units; K and Ra are not. Promotion will often prefer K as an answer result but that can be overridden with ContextUnits as usual. However, that might not be a good idea because some operations are prohibited with affine quantities. Overall, I would say this seems less fiddly and feels right.

I've switched the terminology around (K is an absolute scale). It's late in my timezone so I'm sure there are mistakes to find...

Enables the following: `10*Unitful.ContextUnits(°C,°C) + 2K == (12//1)°C`
- `zero` is supposed to return the same type; not possible for affine quantities. we could consider loosening that and return `zero(10°C) === 0K`, for instance.
- `one` is not allowed since we don't permit multiplication of affine quantities by scalars
- `oneunit` similarly not allowed
@ajkeller34 ajkeller34 changed the title [WIP] Prototyping better temperature handling. [RFC] Prototyping better temperature handling. Oct 31, 2018
docs/src/index.md Outdated Show resolved Hide resolved
src/quantities.jl Outdated Show resolved Hide resolved
@burnpanck
Copy link

burnpanck commented Nov 1, 2018

One comment on the swap of nomenclature: While I appreciate that "absolute Temperature" has a specific meaning in physics, calling temperature points/coordinates in °C relative temperature (as opposed to temperature differences) seems confusing to me. Maybe we should not use relative vs. absolute to refer to the distinction between points (coordinates) and differences (vectors) at all. Maybe use TemperaturePoint or TemperatureCoordinate for those units?

@ajkeller34
Copy link
Collaborator Author

Ok, I've tried to be more explicit to say "relative scale" instead of "relative." I think "relative scale" is commonly used and understood terminology but "relative" is indeed an ambiguous term. I'd like to avoid introducing new jargon though and I worry TemperatureCoordinate may not convey enough information for all to understand.

I will merge once tests pass.

@ajkeller34 ajkeller34 merged commit a7cf8a8 into master Nov 1, 2018
@ajkeller34 ajkeller34 deleted the temperature branch November 1, 2018 20:07
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

5 participants