-
Couldn't load subscription status.
- Fork 50
Add new industry reporter #395
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #395 +/- ##
=======================================
- Coverage 77.1% 73.4% -3.8%
=======================================
Files 250 257 +7
Lines 20198 21451 +1253
=======================================
+ Hits 15589 15754 +165
- Misses 4609 5697 +1088
🚀 New features to boost your workflow:
|
f6a03de to
fb91fd5
Compare
04df012 to
e103f06
Compare
Remove unnecessary reads/writes to disk Replace sequential unit conversions with loop
- Add aggregation step to Final Energy|Industry - Fix |Solids, |Coal and |Biomass aggregate components - Add missing ammonia mass-energy conversion - Fix unit conversion for Final Energy quantities
Only replace Liquids|Other and not Other Sector strings
- Move electricity mapping - Rename function variables for readability
Drop "mode" column in genno Reporter Needs to be dropped to allow for aggregation of methanol production with new meth_h2 modes
- Add Solar to Other Sector total - Add other chemicals to aggregates - Add missing FE HVC total
- Apply code standard to docstrings - Extend type hinting - Chain pandas operations
ffcd011 to
c35c835
Compare
Use "var: in" to align with corresponding basic config file.
- Rename ReporterConfig → Config, move class to its own submodule - Use stdlib dataclasses instead of pydantic. - load_config() wraps Config.from_files(). - Move .reporter_utils.create_var_map_from_yaml_dict() → Config.use_vars_dict() - Move .run_reporting.create_agg_var_map_from_yaml() → Config.use_aggregates_dict(). - Add missing material/report/__init__.py. - Remove pydantic from optional dependencies.
|
|
||
|
|
||
| @dataclass | ||
| class Config: |
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.
@macflo8 this class replaces the pydantic.BaseModel subclass. As well as doing that:
- I collected configuration-handling logic together with the class. This includes:
- Logic formerly in load_config() for locating and handling the format of configuration files.
- Two functions in different modules: reporter_utils.create_var_map_from_yaml_dict() and run_reporting.create_agg_var_map_from_yaml(). These are now closely located with the Config.mapping attribute they are meant to update.
- I added docstrings about the specific data structures that the code expects/can handle, with examples. These should help to maintain the YAML file collection.
test_load_config() still passes; I will monitor CI to see if anything broke. I will also watch for coverage gaps and push ~1 more commit if needed to address those.
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.
Thanks! The new test_run() integration tests marked with XFAIL still needs some adjustment. So that one is failing atm. Since the function errors out early, it does not cover all remaining lines of run_reporting yet. I need to see if it is possible to reach the codecov target with a few smalll changes.
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.
I removed the raises=… keyword from this mark; this way the mark matches regardless of the type of exception raised.
b16c780 to
7bf714d
Compare
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.
Okay, with a few additional test stubs we are just shy of the Codecov thresholds. I don't see us getting much further without a push to build out more complete fixtures, and we've agreed that is out of scope for this PR.
From my POV then all 3 things from my last review are addressed and this is good to merge. Please let me know if you have anything further to push, otherwise I'll merge.
|
Thanks for your contributions and help! I tried to address all other suggestions except for 2 who are not marked as resolved yet. In my view, they can also be tackled in a follow-up PR. I will push one more commit, updating the material docs, since |
|
The two failing CI jobs appear to be flaky (I have seen similar failures unrelated to the current PR/changes, and they occur only on one OS and not others), so I will merge the PR without waiting for them to complete. |
This PR adds an improved industry reporter for MESSAGEix-Materials developed in the course of the ScenarioMIP project. It cherry-picks some initial commits from the #235 branch.
How to review
developer (someone like the reviewer) will be able to understand what the code
does in the future.
PR checklist