Skip to content

Conversation

hwaien
Copy link
Contributor

@hwaien hwaien commented Mar 29, 2020

Seems like Millidegree Celsius is currently missing. This pull request adds support for it.

@angularsen
Copy link
Owner

angularsen commented Mar 29, 2020

Looks good, except you are missing the new properties in the test code. See https://github.com/angularsen/UnitsNet/wiki/Adding-a-New-Unit#4-fix-generated-test-stubs-to-resolve-compile-errors-tests.

@hwaien
Copy link
Contributor Author

hwaien commented Mar 29, 2020

After fixing the tests, I see that my changes result in incorrect behaviors. For example, the round-tripping of Temperature.FromMillidegreesCelsius(kelvin.MillidegreesCelsius).Kelvins does not give the expected result of 1.

Seems like degree Celsius cannot be treated the same way as Meter because the conversion between degree Celsius and the base unit of degree Kelvin is not proportional.

Is the recommended approach to declare millidegree Celsius as its own unit (with its own conversion functions from and to the base unit) side-by-side to degree Celsius?

@codecov-io
Copy link

codecov-io commented Mar 29, 2020

Codecov Report

Merging #771 into master will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
+ Coverage   63.44%   63.52%   +0.07%     
==========================================
  Files         277      277              
  Lines       41055    41135      +80     
==========================================
+ Hits        26049    26129      +80     
  Misses      15006    15006              
Impacted Files Coverage Δ
UnitsNet/GeneratedCode/UnitAbbreviationsCache.g.cs 100.00% <ø> (ø)
...eratedCode/NumberToTemperatureDeltaExtensions.g.cs 100.00% <100.00%> (ø)
...s/GeneratedCode/NumberToTemperatureExtensions.g.cs 100.00% <100.00%> (ø)
UnitsNet/GeneratedCode/Quantities/Temperature.g.cs 56.32% <100.00%> (+0.98%) ⬆️
...Net/GeneratedCode/Quantities/TemperatureDelta.g.cs 61.82% <100.00%> (+5.79%) ⬆️
UnitsNet/GeneratedCode/UnitConverter.g.cs 100.00% <100.00%> (ø)

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 1a4869b...ce6d816. Read the comment docs.

@angularsen
Copy link
Owner

Huh, good question. Temperature units have time and time again proven to be difficult due how they don't share the same zero value and things like arithmetic don't behave like all the other quantities.

I think the solution you came up with is the best compromise, I don't see any other way of getting it added. Good job figuring that out. For me, this is ready to merge. Agreed?

@angularsen
Copy link
Owner

Just a control question, are you sure millidegrees Celsius is used for absolute temperature measurements? My googling seems to indicate it is used for delta/relative temperature measurements (1 millidegree Celsius hotter than X), but I can't immediately find references to absolute measurements (the measured temperature was 10 millidegrees Celsius).

@hwaien
Copy link
Contributor Author

hwaien commented Mar 29, 2020

Good question @angularsen. I am not sure how widespread the practice is, but at my company, millidegrees Celsius is the default unit used by our stakeholders, for both absolute temperature and relative temperature. (For example: heat the substance to an absolute temperature of X millidegrees Celsius, give or take a relative temperature of Y millidegrees Celsius.) For now, we maintain our own conversion functions in conjunction with UnitsNet to meet stakeholder needs.

On a different note: This is my first time contributing to UnitsNet. (What a wonderful project. Thanks to all your hard work!) What's the release process (and timeline) from code changes proposed in a pull request like this one, to the changes being available for consumption from NuGet?

@angularsen
Copy link
Owner

angularsen commented Mar 29, 2020 via email

@angularsen angularsen changed the title Adding support for millidegree Celsius Add MillidegreeCelsius and add prefixes to TemperatureDelta Mar 29, 2020
@angularsen angularsen merged commit 3a1843b into angularsen:master Mar 29, 2020
@angularsen
Copy link
Owner

Nuget on the way
Release UnitsNet/4.55.0 · angularsen/UnitsNet

@hwaien
Copy link
Contributor Author

hwaien commented Mar 30, 2020

Thanks @angularsen !

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.

3 participants