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

Alias fix up #53

Merged
merged 3 commits into from
Jul 9, 2024
Merged

Alias fix up #53

merged 3 commits into from
Jul 9, 2024

Conversation

znicholls
Copy link
Contributor

Turns out that we had accidentally assigned "yr" to be an alias for "a". This meant we had the following behaviour

>>> from openscm_units import unit_registry
>>> val = unit_registry.Quantity(1, "yr")
>>> val
<Quantity(1, 'a')>

This is quite annoying if you need to use string comparisons later (e.g. because you're passing to Fortran).

This PR fixes this so that if you pass in "yr", it stays as yr i.e. you get

>>> from openscm_units import unit_registry
>>> val = unit_registry.Quantity(1, "yr")
>>> val
<Quantity(1, 'yr')>

@znicholls
Copy link
Contributor Author

Surprisingly subtle. Pint's API features once again showing some of their, arguably, less desirable side.

@znicholls
Copy link
Contributor Author

@crdanielbusch @JGuetschow might be of interest for you too as you have to deal with pint sometimes too

Comment on lines +235 to +236
self.define("yr = 1 * year")
self.define("a = 1 * year = annum")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very subtle change. It wasn't obvious to me straight away that 'everything on the right is an aliases for the first thing on the left'

Copy link
Member

@mikapfl mikapfl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite annoying if you need to use string comparisons later (e.g. because you're passing to Fortran).

I don't understand the use-case (don't you have to convert to something explicitly anyway if you need a specific unit string?), but the functionality works as advertised and doesn't seem to break anything.

@znicholls
Copy link
Contributor Author

I don't understand the use-case (don't you have to convert to something explicitly anyway if you need a specific unit string?), but the functionality works as advertised and doesn't seem to break anything.

Bit hard to explain. Let me try again. We pass stuff from Python to Fortran. In Fortran, there is no pint, so we just carry the units round as plain strings. To check that the units are as expected, we just do a basic string comparison. When pint magically converts "yr" to "a", our Fortran-side string comparisons break because they're expecting "yr" but getting "a". We could just change all our Fortran to expect "a", but this magical change feels wrong so I'm trying this instead.

@mikapfl
Copy link
Member

mikapfl commented Jul 9, 2024

But couldn't you run thing.to("unit / yr") before passing thing from python to Fortran?

@znicholls
Copy link
Contributor Author

But couldn't you run thing.to("unit / yr") before passing thing from python to Fortran?

In some cases, but in this case no (can explain on a call one day if you want to dig in further, I'm going to merge as is now though because the change is minor as you say).

@znicholls znicholls merged commit 2bc589b into main Jul 9, 2024
12 checks passed
@lewisjared lewisjared deleted the alias-reversal branch October 23, 2024 22:48
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.

2 participants