Skip to content
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

feat: implement build_priority for core components #13

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

daschw
Copy link
Collaborator

@daschw daschw commented Aug 19, 2024

Motivation

In the CHP addon code we have to check, whether the linked component and the corresponding model variables are already initialized before we can add new constraints refering to both heatz and power component variables.
While this is fine in this specific case, it might become cumbersome for addons that link more than two components.
So it might be useful to have some way of specifying the build order of components in the config.

Changes

This adds the (optional) field build_priority to all core components. Before building the optimization models, all model components are sorted according to their build priority, and components with higher priority are initialized before components with lower priority.
The default priority was set to 0 for all components and 1000 for Decisions.

I will also provide a PR to IESoptLib with changes to the CHP addon using this build priority.
I tested this locally, but ran into #12.
To have this tested properly, I would suggest first having the IESoptLib PR merged and afterwards re-running CI in this PR, but I am open to other suggestions.

Copy link

codecov bot commented Aug 19, 2024

Codecov Report

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

Project coverage is 52.30%. Comparing base (7baf25d) to head (72e9a8e).
Report is 6 commits behind head on main.

Files Patch % Lines
src/core.jl 75.00% 1 Missing ⚠️
@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
+ Coverage   52.27%   52.30%   +0.02%     
==========================================
  Files          99       99              
  Lines        3772     3776       +4     
==========================================
+ Hits         1972     1975       +3     
- Misses       1800     1801       +1     
Files Coverage Δ
src/IESopt.jl 54.19% <100.00%> (-0.15%) ⬇️
src/core/connection.jl 43.18% <ø> (ø)
src/core/decision.jl 46.96% <100.00%> (+0.81%) ⬆️
src/core/node.jl 56.71% <ø> (ø)
src/core/profile.jl 68.00% <ø> (ø)
src/core/unit.jl 87.57% <ø> (ø)
src/core.jl 62.25% <75.00%> (+0.34%) ⬆️

@sstroemer
Copy link
Member

Looks good to me, I'm fine with the test as suggested.

@daschw
Copy link
Collaborator Author

daschw commented Aug 19, 2024

Hm, not sure why the Documentation build now fails, saying the IESoptLib version is fixed to 0.1-0.2.
In the CI workflow IESoptLib 0.3 is used.
I could try to add Pkg.update() to the Documentation workflow in github actions, but I am not sure if we want this.

@sstroemer
Copy link
Member

sstroemer commented Aug 20, 2024

Hm, not sure why the Documentation build now fails, saying the IESoptLib version is fixed to 0.1-0.2. In the CI workflow IESoptLib 0.3 is used. I could try to add Pkg.update() to the Documentation workflow in github actions, but I am not sure if we want this.

Maybe it was generally a bad idea from me to separate those? Since we kinda rely on that alot for tests (therefore later also coverage), we need to update compat, etc. in IESopt.jl anyways every time we introduce something new to IESoptLib.jl. The initial idea was to prevent new core versions just because some template, etc. changed, but... ?

E.g., similar to how Containers is contained in JuMP: https://github.com/jump-dev/JuMP.jl/tree/master/src/Containers

Edit: yea... I already made a stupid mistake because of that: I merged the CompatHelper compat update for IESoptLib - which it detects since it assume 0.x.y to be non-breaking for a change in x, whereas everything else seems to consider "a leading 0 means x can be breaking". So now the general tests are failing because it already pulls v0.3.0 - which I should have thought of but did not.

@daschw daschw merged commit 0b4840a into ait-energy:main Aug 20, 2024
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants