-
Notifications
You must be signed in to change notification settings - Fork 251
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
Adds network energy balance plots #1285
base: master
Are you sure you want to change the base?
Conversation
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.
this looks nice @p-glaum, what is the status? can I support with something?
status hasn't changed so far 😅 . It was just an initial draft that the code is not lost/forgotten. I think it still needs some cleaning and updating because not all graphs look nice at the moment. I collected some TODOs above. |
Validator ReportI am the Validator. Download all artifacts here. General Files comparison
NRMSE: Normalized (combined-min-max) Root Mean Square Error Model Metrics Comparing |
improve plotting script adjust plotting parameters in plotting yaml
I fixed some bugs and made it compatible with the newest master version. For me, it is working and almost all plots look nice. Maybe we can fine tune the config a bit further. This PR is currently not interfering with default snakemake workflow, as you need to call the plot function separately with |
I will have a quick pass over this once @bobbyxng has given his feedback he announced bilaterally. Don't forget to reference the rules in the documentation and add a release note :) |
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.
In my tests, the plots of all carriers ran through @fneum, so the implementation generally looks good to me, only two smaller remarks:
- All plotting-related configs will be moved from config.default.yaml to plotting.default.yaml, as @p-glaum suggested. I support the idea.
vmin
,vmax
, see comment here: Adds network energy balance plots #1285 (comment)
…sv table which was previously not shown
added release note and documentation. There are just two minor open TODOs but you can already take look if you want. |
Closes # (if applicable).
Changes proposed in this Pull Request
This PR is still a draft and needs improvement. Only uses statistics functions to create network plots by including scripts from @bobbyxng, @FabianHofmann and me.
Feel free to make improvements/changes to make it even more modular.
Checklist
envs/environment.yaml
.config/config.default.yaml
.doc/configtables/*.csv
.doc/data_sources.rst
.doc/release_notes.rst
is added.TODOS
branch_factor