kbase / sample_service

Service for creating, modifying, and retrieving samples
MIT License
0 stars 11 forks source link

Issue with Units around temperature #387

Open slebras opened 3 years ago

slebras commented 3 years ago

I've had issues with unit expressions around temperature. The error is thrown on this line

https://github.com/kbase/sample_service/blob/695bb800cb3babe2084e93c260affcd10018d3e7/lib/SampleService/core/validator/builtin.py#L222

the error is: pint.errors.OffsetUnitCalculusError: Ambiguous operation with offset unit (degree_Celsius, ). See https://pint.readthedocs.io/en/latest/nonmult.html for guidance.

The multiplication on the line shared above between units and 1 does not work with temperatures in pint. "These temperature units are expressed in a system with a reference point, and relations between temperature units include not only a scaling factor but also an offset." in their words.

MrCreosote commented 3 years ago

To reproduce:

In [1]: import pint                                                             

In [2]: ur = pint.UnitRegistry()                                                

In [3]: unit = ur.parse_expression('degF')                                      

In [4]: 1 *  unit                                                               
---------------------------------------------------------------------------
OffsetUnitCalculusError                   Traceback (most recent call last)
<ipython-input-4-69f3d3a309b4> in <module>
----> 1 1 *  unit
*snip*

OffsetUnitCalculusError: Ambiguous operation with offset unit (degree_Fahrenheit, ). See https://pint.readthedocs.io/en/latest/nonmult.html for guidance.
MrCreosote commented 3 years ago
In [7]: unit = ur.parse_expression('degK')                                      

In [8]: 1 * unit                                                                
Out[8]: 1 <Unit('kelvin')>
MrCreosote commented 3 years ago

How is this unit test passing? https://github.com/kbase/sample_service/blob/master/test/core/validator/builtin_test.py#L178

MrCreosote commented 3 years ago

Looks like the incoming units have to be absolute. Does it work with K or R?

MrCreosote commented 3 years ago

https://pint.readthedocs.io/en/stable/nonmult.html

MrCreosote commented 3 years ago
As an alternative to raising an error, pint can be configured to work more relaxed via setting the UnitRegistry parameter autoconvert_offset_to_baseunit to true. In this mode, pint behaves differently:

    Multiplication of a quantity with a single offset unit with order +1 by a number or ndarray yields the quantity in the given unit.

>>> ureg = UnitRegistry(autoconvert_offset_to_baseunit = True)
>>> T = 25.4 * ureg.degC
>>> T
<Quantity(25.4, 'degree_Celsius')>
MrCreosote commented 3 years ago

So a couple potential fixes, I think: 1) Only allow absolute temperature units in input, and throw a better error if they're not. 2) The option above, which probably needs to be understood better before implementation.

slebras commented 3 years ago

I'm leaning towards option 2 above, If we only ever use units * 1, it should not effect the incoming data. I also don't think we should alter any of the data, outside of unit conversions, within the SampleService, so that shouldn't be an issue going forward

slebras commented 3 years ago

PR addressing this: https://github.com/kbase/sample_service/pull/388

eapearson commented 2 years ago

Another solution, with no other side effects, is to use the quantity api.

pint.quantity.Quantity(1, unit).to(req_unit)

which is utilized in https://github.com/kbase/sample_service/pull/410