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

Add VariableCodeList.units attribute and tests #289

Merged

Conversation

danielhuppmann
Copy link
Member

I just ran into a situation where I wanted to get all units from a VariableCodeList, so this PR adds an attribute for that...

Open question: going back and forth between set and list seems to make the order arbitrary, but allowing "None" (for dimensionless variables) also preempts ordering easily. One could simply drop "None" from that list (given that it's not a unit) or we could always add it at the end or replace by "dimensionless" (this is what pint does)...

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Looks good to me.
Two small comments below.
Regarding your questions, I would replace None with "dimensionless". Also you could use a frozenset as the return value. This way you could ensure that the value are unique and ordered.

else:
unit_set.add(variable.unit)

return list(unit_set)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to return a list? Wouldn't it work as well if you just returned the set?

Copy link
Member Author

Choose a reason for hiding this comment

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

Call me old-fashioned, but I have cast all return-objects in pyam (index dimensions, etc.) to lists for consistency...

nomenclature/codelist.py Outdated Show resolved Hide resolved
@danielhuppmann
Copy link
Member Author

Regarding your questions, I would replace None with "dimensionless".

On second thought, I would actually not follow pint (use the string dimensionless) but follow pyam (using an empty string). This is closer to what a user will find in the yaml files, and it also always comes first in the ordering...

@phackstock
Copy link
Contributor

On second thought, I would actually not follow pint (use the string dimensionless) but follow pyam (using an empty string). This is closer to what a user will find in the yaml files, and it also always comes first in the ordering...

Sounds good as well. I agree with your point that it's more in line with closely reproducing what's the yaml files.

Copy link
Contributor

@phackstock phackstock left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@danielhuppmann danielhuppmann merged commit d592708 into IAMconsortium:main Nov 9, 2023
8 checks passed
@danielhuppmann danielhuppmann deleted the feature/list-of-units branch November 9, 2023 09:01
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