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

Higher-order unit conversion with GWP context #36

Open
lewisjared opened this issue May 24, 2022 · 5 comments
Open

Higher-order unit conversion with GWP context #36

lewisjared opened this issue May 24, 2022 · 5 comments

Comments

@lewisjared
Copy link
Contributor

Describe the bug

Unit conversion fails when using a GWP context for units that have higher-order dimensionality. For example, one cannot convert kg CO2/EJ to kg CH4/EJ, but kg CO2/yr => kg CH4/yr works.

The above example fails with the following message:

Cannot convert from 'CO2 * metric_ton / exajoule' ([carbon] * [time] ** 2 / [length] ** 2) to 'CH4 * metric_ton / exajoule' ([methane] * [time] ** 2 / [length] ** 2)

Maybe I'm doing something wrong, but the dimensions match (except for carbon=>CH4 which should be handled by the GWP context).

Failing Test

def test_multidimensional_with_context():
    with unit_registry.context("AR6GWP100"):
        unit_registry("t CO2/EJ").to("t CH4/EJ")

This test fails with the following exception message:

test_units.py::test_multidimensional_with_context FAILED                 [100%]
tests/unit/test_units.py:373 (test_multidimensional_with_context)
def test_multidimensional_with_context():
        with unit_registry.context("AR6GWP100"):
>           unit_registry("t CO2/EJ").to("t CH4/EJ")

test_units.py:376: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../venv/lib/python3.9/site-packages/pint/quantity.py:716: in to
    magnitude = self._convert_magnitude_not_inplace(other, *contexts, **ctx_kwargs)
../../venv/lib/python3.9/site-packages/pint/quantity.py:665: in _convert_magnitude_not_inplace
    return self._REGISTRY.convert(self._magnitude, self._units, other)
../../venv/lib/python3.9/site-packages/pint/registry.py:1044: in convert
    return self._convert(value, src, dst, inplace)
../../venv/lib/python3.9/site-packages/pint/registry.py:1959: in _convert
    return super()._convert(value, src, dst, inplace)
../../venv/lib/python3.9/site-packages/pint/registry.py:1566: in _convert
    return super()._convert(value, src, dst, inplace)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <openscm_units._unit_registry.ScmUnitRegistry object at 0x7fb8bf642370>
value = 1.0, src = <UnitsContainer({'CO2': 1, 'exajoule': -1, 'metric_ton': 1})>
dst = <UnitsContainer({'CH4': 1, 'exajoule': -1, 'metric_ton': 1})>
inplace = False, check_dimensionality = True

    def _convert(self, value, src, dst, inplace=False, check_dimensionality=True):
        """Convert value from some source to destination units.
    
        Parameters
        ----------
        value :
            value
        src : UnitsContainer
            source units.
        dst : UnitsContainer
            destination units.
        inplace :
             (Default value = False)
        check_dimensionality :
             (Default value = True)
    
        Returns
        -------
        type
            converted value
    
        """
    
        if check_dimensionality:
    
            src_dim = self._get_dimensionality(src)
            dst_dim = self._get_dimensionality(dst)
    
            # If the source and destination dimensionality are different,
            # then the conversion cannot be performed.
            if src_dim != dst_dim:
>               raise DimensionalityError(src, dst, src_dim, dst_dim)
E               pint.errors.DimensionalityError: Cannot convert from 'CO2 * metric_ton / exajoule' ([carbon] * [time] ** 2 / [length] ** 2) to 'CH4 * metric_ton / exajoule' ([methane] * [time] ** 2 / [length] ** 2)

../../venv/lib/python3.9/site-packages/pint/registry.py:1077: DimensionalityError

Expected behavior

Units are able to be converted

System (please complete the following information):
Ubuntu
Python 3.9
commit f97fac6

@znicholls @rgieseke

@znicholls
Copy link
Contributor

And so our hacking comes back to bites us. I think it's all related to how we define these conversions, we basically have to define composite changes too like here

def _add_transformations_to_context(

Option a: Add these energy type conversions too
Option b: Re-write the repository to handle this in a way that better respects SI units
Option c: Go digging in pint to work out why it can't seem to figure this out for itself (it doesn't completely make sense to me that it can't handle this once we've added the new base unit).

I would go for option a as it is simplest. Option c would be a good alternative.

@znicholls
Copy link
Contributor

I think it's an issue in pint though, the way it does its search for a way between the two units is weird in a context. It basically searches for the entire unit, not just the bit that is actually covered by the context. I don't know how easy or otherwise that is to fix.

https://github.com/hgrecco/pint/blob/373490eb790d100915677b63a0aaa36816f10449/pint/util.py#L275

@znicholls
Copy link
Contributor

@lewisjared
Copy link
Contributor Author

Good detective work.

Option a would be the simplest given that there is already functionality to build the unit graph. The downside is that we can't capture all permutations that could be useful and the error message isn't exactly helpful. But adding a few extras shouldn't hurt?

Do you see an easy way to detect this situation and provide a better error message/raise a different exception?

@znicholls
Copy link
Contributor

But adding a few extras shouldn't hurt?

Ye definitely fine until we work out a longer term solution

Do you see an easy way to detect this situation and provide a better error message/raise a different exception?

It's probably possible to do something with base units (pint has some functionality to show the difference) and then if they're both units we've defined say something like 'check compound units' but it would take some fiddling I suspect

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

No branches or pull requests

2 participants