Conversation
There was a problem hiding this comment.
Pull request overview
This PR integrates investment cost functionality into the bioCHP model by utilizing the previously unused C_inv parameter. The changes enable investment modeling for bioCHP nodes by switching from operational to investment-based model types.
- Updated bioCHP model to incorporate investment data structures using the
C_invparameter - Changed test model type from
OperationalModeltoInvestmentModelwith discount rate - Adjusted test expectations to reflect the behavioral changes from investment modeling
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/EnergyModelsLanguageInterfaces.jl | Added import for EnergyModelsInvestments module |
| src/datastructures.jl | Modified BioCHP constructor to create investment data using C_inv and set initial capacity to zero |
| test/utils.jl | Changed model type from OperationalModel to InvestmentModel with 7% discount rate |
| test/test_bioCHP.jl | Updated test assertions for emissions and deficits to match new investment model behavior |
| test/runtests.jl | Refactored include paths to use joinpath for cross-platform compatibility |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…nalia_Solar-Energy-Model
…713678800501e-15 > 0 triggers the bioCHP sink test
JulStraus
left a comment
There was a problem hiding this comment.
Looking through it, two things come directly to my mind:
- this PR enforces that we always load
EnergyModelsInvestmentswhen loadingEMLI. Otherwise, we might potentially run into problems when the model is created? As I do not have a proper C++ compiler on my PC, can you test locally what happens in an environment in which you haveEMLI,TS, andEMBloaded, but notEMI. Is it working in this situation to build and optimize a problem? - The max installed capacity is based on the specified electric capacity and we allow for
ContinuousInvestmentsto reach the capacity. This is essentially a limitation of the current approach.
I honestly do not know a good approach for the implementation right now, but I would also not like to rush the things pre Christmas to get it implemented. there are quite a few things which should be solved differently, see my individual comments.
My suggestion is hence to:
- comment out the lines changed for CHP (l. 864-875) and revert the capacity line and
- identify whether
C_invand specify in this case, if the costs are absolute or relative.
I would then create a different constructor in an EnergyModelsInvestments extension in which we also specify the initial capacity and the desired capacity for the investment data. But that can wait until after the new year.
JulStraus
left a comment
There was a problem hiding this comment.
I think my previous comment was a bit misunderstood. I did not mean that we should only have the constructor in the EMI extension. Instead, I wanted a second constructor in the EMI extension which allows for the case with investments.
This would the correspond to the following constructor within src/datastructures.jl
function BioCHP(
id::Any,
cap::TimeProfile,
mass_fractions::Dict{<:ResourceBio,<:Real},
heat_output_ratios::Dict{<:ResourceHeat,<:Real},
electricity_resource::Resource;
data::Vector{Data} = Data[],
libpath::String = joinpath(
@__DIR__,
"..",
"..",
"CHP_modelling",
"build",
"lib",
"libbioCHP_wrapper.so",
),
)and the following constructor within ext/EMIExt/EMIExt.jl
function EMLI.BioCHP(
id::Any,
cap_invest::TimeProfile,
cap_init::TimeProfile,
mass_fractions::Dict{<:ResourceBio,<:Real},
heat_output_ratios::Dict{<:ResourceHeat,<:Real},
electricity_resource::Resource;
data::Vector{Data} = Data[],
libpath::String = joinpath(
@__DIR__,
"..",
"..",
"CHP_modelling",
"build",
"lib",
"libbioCHP_wrapper.so",
),
)I just realized now that we must in this situation specify cap_init and cannot have it as a keyword argument as it would otherwise not work...
This implies that everything excluding the return statement and the creation of the investment data can be a subfunction which is called by both constructors.
JulStraus
left a comment
There was a problem hiding this comment.
Looks good to me now. Please do a squash and merge with a change to the commit message (e.g., I do not think my commits must be included) after the CI went successful.
This PR fixes missing usage of the C_inv parameter (investment cost) from the bioCHP model. The tests are adjusted accordingly.
Also, fixed test to avoid machine epsilon precision inaccuracies, e.g., 3.552y13678800501e-15 > 0 triggers the bioCHP sink test.
Also enable CSPandPV-node usage for power_thermal=0 or power_pv=0 from Tecnalia_Solar-Energy-Model.