Hierarchy Filter (first pull request) #226
Conversation
phackstock
requested review from
danielhuppmann and
phackstock
and removed request for
danielhuppmann
|
Thanks for the initial PR, @GretchenSchowalter! To directly answer your first question, I think that ideally, the filter-option should return a |
The hierarchy filter now returns an instance of the RegionCodeList
|
Okay! Thanks for the feedback! I've updated the code, so I will make another pull request soon. |
|
No need to close the PR and start a new one, @GretchenSchowalter - simply add new commits to this branch… |
Fixing errors with Stickler
Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
changing name from "hierarchy_filter" to "filter" Co-authored-by: Daniel Huppmann <dh@dergelbesalon.at>
changed the "hierarchy_filter()" to just "filter()"
There was a problem hiding this comment.
Looks quite good to me, nice work @GretchenSchowalter.
Just two minor comments below.
Co-authored-by: Philip Hackstock <20710924+phackstock@users.noreply.github.com>
Co-authored-by: Philip Hackstock <20710924+phackstock@users.noreply.github.com>
adding list of available filter hierarchy options to the value error, and changing the name of the new RegionCodeList instance to self.name (name of the original RegionCodeList getting filtered).
There was a problem hiding this comment.
Looks good to me, thanks @GretchenSchowalter.
The one open question is if we should add this new feature formally to the docs or if this is more of an internal one anyway. @danielhuppmann, what are your feelings on that?
|
If I see it correctly, the RegionCodeList class is not yet part of the API docs, like https://github.com/IAMconsortium/nomenclature/blob/main/doc/source/api/codelist.rst It would be sufficient to add it there, I think.. |
There was a problem hiding this comment.
Thanks for moving forward, @GretchenSchowalter - a few more suggestions inline.
There was a problem hiding this comment.
Looks good to me! Thanks @GretchenSchowalter, and congrats for your first successful PR!
I've added the hierarchy filter to the codelist.py file, and two different tests to the test_codelist.py.
Short explanation: The filter requires two arguments: self and the particular hierarchy you want to filter for. The first step in the function is the creation of an empty list called countries. Then iterating through the values of the self.mapping.values(), we then can compare if the hierarchy for each region matches the hierarchy argument. If so, then the name of that country is appended to the countries[] list. Once the computer has iterated through self.mapping.values(), it then checks if the countries[] list is empty. If not, then filter has worked and it returns the list of names. If the countries[] list is still empty, then either the hierarchy argument inputted is not used for the particular model, or the hierarchy argument has a typo.
I have just a couple questions:
Right now the hierarchy_filter(hierarchy: str) returns a List[str] with just the names of each of the regions that compose that particular hierarchy. Would it be better if the filter instead returned List[Code]?
Would it be helpful to add another level of filtration? For example, print(hierarchy_filter(R5)) prints OECD & EU, Asia, Reforming economies, Latin America, Middle East, and Other. Then one step further, print(hierarchy_filter(OECD & EU)) prints Australia, Austria, Belgium,..., United States.