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 a general-config feature to add all countries to RegionCodeList #262

Merged

Conversation

danielhuppmann
Copy link
Member

This PR adds a simple way to add all countries to a RegionCodeList directly from the pycountry package, using a "general-config" yaml file.

This implements the first idea of #261.

@danielhuppmann
Copy link
Member Author

When looking at the pycountry package in more detail for the SSP review process, I realized that it uses some country-names that are not consistent with the use in our community. So I guess that we will have to hard-code the country-names and ISO3-codes after all...

@danielhuppmann
Copy link
Member Author

After looking at the pycountry package some more, I suggest (and implemented) a mapping of "country-name-overrides" where we explicitly depart from pycountry standards. Seems like a preferable option than to duplicate an entire list of country names and ISO2/ISO3 codes...

The model-registration xlsx template has been updated accordingly.

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.
The only comment I'd have is on the use of the DataStructureConfig.
Right now its just a wrapper around a dictionary. I think you could save a lot of manual checking, for now and for the future, if you completely defined the structure of the DataStructureConfig and its dimension specific settings.
My inline comments are regarding that.

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

Implemented the suggested changes, @phackstock, towards nested pydantic classes. And I also added some basic documentation - I'm planning to add more documentation in a follow-up PR, then we can do a release 0.11

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.
The pydantic config classes make me very happy, thank you :)
One question that came to my mind was if it would be easier to just work with ISO codes directly as the region names. Might be easier for modeling teams to align their outputs.

@danielhuppmann
Copy link
Member Author

One question that came to my mind was if it would be easier to just work with ISO codes directly as the region names. Might be easier for modeling teams to align their outputs.

Right, @willu47 made a similar point here: OSeMOSYS/osemosys2iamc#33 (comment)

But I don't really like the idea for several reasons:

  • ISO3 codes are not really user-friendly - it's difficult to get a quick intuition in a tool like Artelys' FutureSight or the Scenario Explorer, so we would probably invent some "conversion tool" again
  • ISO3 codes are not fully universal either, e.g., Kosovo doesn't have one
  • Using ISO3 codes in native-country naming also has issues, for example REMIND 3.2 uses "UKI" internally for "United Kingom and Ireland" - this can be very confusing when users try an ISO3-mapping on that... (see here).

So I think it's better to nudge modelers to use intelligible names instead of abbreviations.

Nonetheless, we should add a simple ISO2/ISO3 mapping tool to the shortened country names added in this PR (and write documentation for it).

@phackstock
Copy link
Contributor

But I don't really like the idea for several reasons:

Ah right, good points, it's never as simple as you think ...

Nonetheless, we should add a simple ISO2/ISO3 mapping tool to the shortened country names added in this PR (and write documentation for it).

Good point, that would be surely appreciated. In the context of NGFS, the users were specifically asking for the country level data to be reported in ISO3 so that would come in handy there.

@danielhuppmann danielhuppmann merged commit 7613986 into IAMconsortium:main Jul 13, 2023
@danielhuppmann danielhuppmann deleted the feature/pycountry-definition branch July 13, 2023 09:49
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