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

MetaCode and MetaCodeList classes with allowed_values attribute #246

Merged
merged 8 commits into from
May 12, 2023

Conversation

GretchenSchowalter
Copy link
Contributor

Hello! This is my first attempt at addressing issue #241. I have added MetaCode and MetaCodeList classes, where MetaCode has the attribute allowed_values. Within the MetaCodeList class, there is a method from_yaml_files() (which is very similar to the from_directory() method in RegionCodeList) which reads the .yaml files and creates a MetaCodeList. I've also added two very simple tests. I wasn't really sure how to guarantee that the from_yaml_files() method read from a folder called meta, so I would be grateful for a suggestion!

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Thanks @GretchenSchowalter, a few suggestions inline...

validation_schema: ClassVar[str] = "meta indicators"

@classmethod
def from_yaml_files(cls, name: str, path: Path, file_glob_pattern: str = "**/*"):
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep the from_directory() convention here? I think we should keep the meta-indicators in a "meta" folder for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

I second that one it should definitely be called from_directory() otherwise DataStructureDefinition cannot use it.
Probably should implement a MetaCodeList class that enforces that ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this again, I don't think the method is actually needed at all. Since MetaCodeList inherits from CodeList and CodeList implements from_directory we should be good.



def test_MetaCode_allowed_values_attribute():
reg = MetaCode(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
reg = MetaCode(
meta = MetaCode(

"""
# TODO write all dimensions to the file
self.variable.to_excel(excel_writer, sheet_name, sort_by_code, **kwargs)
import logging
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why GitHub thinks this entire file was changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering the same, might even be a GitHub issue.

from nomenclature.validation import validate

logger = logging.getLogger(__name__)
SPECIAL_CODELIST = {"variable": VariableCodeList, "region": RegionCodeList}
Copy link
Member

Choose a reason for hiding this comment

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

Please add the MetaCodeList here.

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.

Thanks @GretchenSchowalter for starting this work on the MetaCodes.
Also a few suggestions from my side.

nomenclature/code.py Outdated Show resolved Hide resolved
nomenclature/code.py Show resolved Hide resolved
nomenclature/codelist.py Outdated Show resolved Hide resolved
nomenclature/codelist.py Show resolved Hide resolved
tests/test_code.py Show resolved Hide resolved
tests/data/meta/allowed_values_2.yaml Outdated Show resolved Hide resolved
nomenclature/codelist.py Outdated Show resolved Hide resolved
@GretchenSchowalter
Copy link
Contributor Author

Hello! Thanks for all your feedback. I decided to try getting rid of the method I created in the MetaCodeList class and just use the from_directory() from the CodeList class, but to do so I had to make a few adjustments. For example, making sure that from_directory() creates a list of MetaCodes, rather than a list of Codes. I also had to create a meta_schema.yaml file for the validate() to work within the from_directory() method.

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.

Sorry for the schema confusion.
@danielhuppmann please correct me if you're of a different opinion but I'd vote to remove all the schemas anyway soon. @GretchenSchowalter for some context, this was an early attempt at defining the file structures for codelists. The idea is that the yaml file is validated against the schema before it is even parsed into the codes.
In reality this never added that much value since a faulty code list would just fail at a different point and the information given by the validation schema was not any better than the one from pydantic.
I'd vote to remote the schema from this PR, after that it's good to be merged from my side.

nomenclature/validation_schemas/meta_schema.yaml Outdated Show resolved Hide resolved
nomenclature/codelist.py Outdated Show resolved Hide resolved
nomenclature/codelist.py Outdated Show resolved Hide resolved
nomenclature/codelist.py Outdated Show resolved Hide resolved
nomenclature/codelist.py Outdated Show resolved Hide resolved
@GretchenSchowalter GretchenSchowalter marked this pull request as ready for review May 12, 2023 08:29
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.

Good to be merged, from my side, thanks @GretchenSchowalter. 👍.

Copy link
Member

@danielhuppmann danielhuppmann left a comment

Choose a reason for hiding this comment

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

Nice work, thanks!

@danielhuppmann danielhuppmann merged commit ed6baf3 into IAMconsortium:main May 12, 2023
@GretchenSchowalter GretchenSchowalter deleted the MetaCode branch May 12, 2023 11:23
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.

3 participants