-
Notifications
You must be signed in to change notification settings - Fork 10
Initially implement CMEC under cmip_ref_core #84
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
Conversation
lewisjared
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are an initial set of suggestions
packages/ref-core/src/cmip_ref_core/pycmec/tests/unit/test_cmec_metric.py
Outdated
Show resolved
Hide resolved
packages/ref-core/src/cmip_ref_core/pycmec/tests/unit/test_cmec_metric.py
Outdated
Show resolved
Hide resolved
packages/ref-core/src/cmip_ref_core/pycmec/tests/unit/test_cmec_metric.py
Outdated
Show resolved
Hide resolved
packages/ref-core/src/cmip_ref_core/pycmec/tests/unit/test_cmec_metric.py
Outdated
Show resolved
Hide resolved
|
Ahh I see that you ran |
| "name": "National Biomass and Carbon data set for the Year 2000", | ||
| }, | ||
| }, | ||
| "log": "cmec_output.log", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nocollier As mentioned on slack the CMEC output bundle has a log field which takes a log file
Codecov ReportAll modified and coverable lines are covered by tests ✅ see 42 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@lewisjared I plan to add the changelog, so which one shall I use, "feature" or "improvement"? Thanks |
This is a new |
… in each level of RESULTS tree
|
Nice stuff @minxu74. I think all that remains is adding a dependency on pydantic to the
|
|
Would you like to get this merged as is, and then do any follow up later. It is pretty self contained at the moment, but you would have to merge main back into this branch if you wanted to work on any other parts of the codebase as they have likely changed since you made this branch |
|
@lewisjared I plan to submit another two commits tonight to address your suggestions and add more comments on the arguments of class methods. After that, if you and @acordonez agree, I can merge it to the main first as it does not interact with other ref components yet. We could create another PR to integrate the CMEC validators on other ref components. I wonder if it works? |
|
I think that is a good idea. We are still actively building so I don't mind if this isn't 100% yet as it's more important to see how it works with the metrics providers |
Implement CMEC output and metric bundle standards using pydantic
Checklist
Please confirm that this pull request has done the following:
changelog/Partially address the issue #70