-
Notifications
You must be signed in to change notification settings - Fork 2
Fix miscellaneous bugs and miscellaneous improvements #22
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
…but when get_products(system) is not a subset of keys(id_to_color_map). This case did not expand id_to_color_map correctly.
…rogen and add a colors for the HeatLT and HeatHT resources.
…for countries just above the ocean layer.
…ates are not provided).
JulStraus
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.
Looks generally speaking good. Please take a look at the suggestion for the descriptive names.
Have you tested that it works now with length(get_products(system)) == length(id_to_color_map)? If so, I think it would be good to have that as well included in the test setup.
src/descriptive_names.yml
Outdated
| CycleLife: | ||
| stack_cost: "Relative cost for replacing a battery stack" | ||
|
|
||
| ## EnergyModelsHeat |
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.
No ThermalEnergyStorage? Probably ok with the base as the other parameters are no TimeProfiles
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 will add ThermalEnergyStorage.
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.
Please just check that it is necessary. Based on my understanding, it should not be.
Co-authored-by: Julian Straus <104911227+JulStraus@users.noreply.github.com>
…EnergyModelsGUI.jl into fix/examples_bug_and_misc
Zetison
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.
Corrections to come in next push.
src/descriptive_names.yml
Outdated
| CycleLife: | ||
| stack_cost: "Relative cost for replacing a battery stack" | ||
|
|
||
| ## EnergyModelsHeat |
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 will add ThermalEnergyStorage.
I suggest adding this test later to get this PR registered asap. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
JulStraus
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.
Looks ok to me. I would suggest squash and merge. We can at a latter point also consider to remove all fields that are not relevant in descriptive_names.yml as we currently still have their a mix of fields included.
This PR makes the setup of tailored
id_to_color_mapmore robust, adds a missing docstring description of the optional argumentcase_namein theGUI-function and adds the missing EnergyModelsGUI package from the Project.toml file in the examples folder. Additionally, the following is adjusted and improved:EnergyModelsHeatandEnergyModelsHydrogen, and added colors for theHeatLTandHeatHTresources.