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

Wrong calculation when using temperature prefixes #1381

Closed
Christian-Mueller opened this issue Sep 2, 2021 · 6 comments
Closed

Wrong calculation when using temperature prefixes #1381

Christian-Mueller opened this issue Sep 2, 2021 · 6 comments
Labels

Comments

@Christian-Mueller
Copy link

I'm doing following conversion:

from pint import UnitRegistry

ureg = UnitRegistry()
t = ureg.Quantity(20, ureg.degC)
print(t.to(ureg.millidegC))

output:
293150.0 millidegree_Celsius

It seems like prefixes for degree Celsius are not handled correctly. The result should be 20000 millidegree_Celsius.
Looks like it changes the unit to Kelvin internally.
It this a known issue? Is there a workaround for my use case?

@jules-ch jules-ch added the bug label Sep 2, 2021
@jules-ch
Copy link
Collaborator

jules-ch commented Sep 3, 2021

It seems millidegree_Celsius is not identified as offset units in the convert stage hence the value does not make sense afterwards.

I'll investigate

@Christian-Mueller
Copy link
Author

Any progress so far on this bug?

@jules-ch
Copy link
Collaborator

jules-ch commented Oct 5, 2021

The issue has been identified but the implementation is tricky with the current implementation:

pint/pint/registry.py

Lines 1529 to 1550 in c99ed1f

if src_offset_unit:
value = self._units[src_offset_unit].converter.to_reference(value, inplace)
src = src.remove([src_offset_unit])
# Add reference unit for multiplicative section
src = self._add_ref_of_log_unit(src_offset_unit, src)
# clean dst units from offset units
if dst_offset_unit:
dst = dst.remove([dst_offset_unit])
# Add reference unit for multiplicative section
dst = self._add_ref_of_log_unit(dst_offset_unit, dst)
# Convert non multiplicative units to the dst.
value = super()._convert(value, src, dst, inplace, False)
# Finally convert to offset units specified in destination
if dst_offset_unit:
value = self._units[dst_offset_unit].converter.from_reference(
value, inplace
)
return value

Here the conversion for offset untis is handled like so:

  • Value is converted to the reference unit of the source unit which is Kelvin for all temperature unit
  • Conversion for remaining unit is done for non offset dimensions
  • Value is converted to destination unit from reference unit of destination unit which is Kelvin for all temperature unit

In the case of prefixed units:
for DegC to mDegC

  • Value is converted to the reference unit which is kelvin
  • Conversion for remaining unit is done for non offset dimensions
  • Value is converted from reference unit which is DegC for mDegC

calculation does output the wrong value, for those steps

  • 20 -> 293.15 (OffsetConverter for DegC)
  • 293.15 -> 293.15 (nothing happens no other units)
  • 293.15 -> 293150.0 (ScaleConverter of prefixed unit for mDegC)

I'll probably end up with a fix for only those prefixed temperature units.

Thoughts @hgrecco?

I find it difficult to work with UnitDefinition, UnitsContainer there

@hgrecco
Copy link
Owner

hgrecco commented Oct 7, 2021

At the very beginning of Pint (before offset units were implemented) there was a proposal to to write a special converter for ocassions in which prefix was removed, added or change. This was dropped as unnecesary in that case and would have helped here.

However, I do think there is a deeper thing to discuss. IMHO si-prefixing offset units is not a good practice. It is ok to express an interval (in which case the temperature becomes a multiplicative unit anyway). And is is kind of ok to write a value, but we should be careful when operating (converting, arithmetics, etc) with this value.

@dalito
Copy link
Contributor

dalito commented Nov 6, 2021

I also thought a bit about this: mDegC is the same as 1e-3 * degC which is a multiplication that is forbidden by default. So to be consistent pint should in my opinion raise errors for

  1. any kind of conversion to an SI-prefixed offset unit and
  2. creation of quantities with an SI-prefix (ureg.Quantity(20, ureg.milli_degC)).

I would vote for raising errors even with ureg.autoconvert_offset_to_baseunit = True.

@hgrecco
Copy link
Owner

hgrecco commented Apr 27, 2023

Moving this to #1754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants