Skip to content

Fix miscellaneous bugs and miscellaneous improvements#22

Merged
Zetison merged 15 commits intomainfrom
fix/examples_bug_and_misc
Jun 3, 2025
Merged

Fix miscellaneous bugs and miscellaneous improvements#22
Zetison merged 15 commits intomainfrom
fix/examples_bug_and_misc

Conversation

@Zetison
Copy link
Collaborator

@Zetison Zetison commented Jun 2, 2025

This PR makes the setup of tailored id_to_color_map more robust, adds a missing docstring description of the optional argument case_name in the GUI-function and adds the missing EnergyModelsGUI package from the Project.toml file in the examples folder. Additionally, the following is adjusted and improved:

  • Added more descriptive names for EnergyModelsHeat and EnergyModelsHydrogen, and added colors for the HeatLT and HeatHT resources.
  • Order the colors (by id) in the Resources legend.
  • Move boundaries for countries just above the ocean layer.
  • Fix default placements of the nodes in a uniform circle (when coordinates are not provided).
  • Only create the design-folder if needed.

@Zetison Zetison requested a review from JulStraus June 2, 2025 09:18
@Zetison Zetison self-assigned this Jun 2, 2025
@Zetison Zetison added bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request labels Jun 2, 2025
Copy link
Member

@JulStraus JulStraus left a 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.

CycleLife:
stack_cost: "Relative cost for replacing a battery stack"

## EnergyModelsHeat
Copy link
Member

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add ThermalEnergyStorage.

Copy link
Member

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.

Jon Vegard Venås and others added 4 commits June 2, 2025 13:48
Copy link
Collaborator Author

@Zetison Zetison left a 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.

CycleLife:
stack_cost: "Relative cost for replacing a battery stack"

## EnergyModelsHeat
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I will add ThermalEnergyStorage.

@Zetison
Copy link
Collaborator Author

Zetison commented Jun 2, 2025

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.

I suggest adding this test later to get this PR registered asap.

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/utils_gen/structures_utils.jl 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files with missing lines Coverage Δ
src/setup_GUI.jl 98.80% <100.00%> (+0.02%) ⬆️
src/setup_topology.jl 98.24% <100.00%> (-0.04%) ⬇️
src/utils_gen/structures_utils.jl 94.26% <66.66%> (-0.78%) ⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@JulStraus JulStraus left a 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.

@Zetison Zetison merged commit 4399f17 into main Jun 3, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants