spine-tools / SpineOpt.jl

A highly adaptable modelling framework for multi-energy systems
https://www.tools-for-energy-system-modelling.org/
GNU General Public License v3.0
58 stars 13 forks source link

Unit flow upper bounds are not correctly calculated when using replacement expressions #1056

Closed nhniina closed 2 months ago

nhniina commented 3 months ago

Replacement expressions seem to lead to a too small unit flow upper bound, and potentially an infeasible model. It seems that parameters fix_ratio_in_out_unit_flow, fix_units_on_coefficient_in_out and unit_start_flow are not all correctly considered in the upper bounds. In my test, only fix_ratio_in_out_unit_flow has an effect on the upper bound.

Running SpineOpt with this input data JSON should reproduce the bug.

The input data sets fix_ratio_in_out_unit_flow = 1.66, fix_units_on_coefficient_in_out = 6.27 and unit_start_flow = 1000.0. The capacity of to_node = 100.0 and number_of_units = 1.0.

One would expect to see the following result for the upper bound of from_node unit_flow: 1000 + 6.27 + 1.66*100 = 1172.27. Instead, I get an upper bound of 166.

The model works as expected when using unit_incremental_heat_rate and unit_idle_heat_rate instead (scenario heatrate instead of Base). The model also works as expected when disabling replacement expressions in functions constraint_ratio_unit_flow_indices(m::Model, ratio) and add_variable_unit_flow!(m::Model). (In both of the cases that work, no upper bound for from_node unit_flow is created, only for to_node unit_flow.)

These are the resulting LP files:

SpineOpt version: 76bd7266bb85adbf782e9652393fdf863d523e57

Tasqu commented 3 months ago

This is still a WIP, but with @nhniina we think the issue is in the variable_common.jl lines 85-88:

    inverse_replacement_expressions = Dict(
        ref_ind => (ind, 1 / coeff)
        for (ind, (ref_ind, coeff)) in ((ind, ref[name]) for (ind, ref) in replacement_expressions)
    )

where the name limits the variables that are accounted for so that the full replacement_expression is not accounted for. Using

    inverse_replacement_expressions = Dict(
        ref_ind => (ind, 1 / coeff)
        for (ind, var_dict) in replacement_expressions
        for (var_name, (ref_ind, coeff)) in var_dict
    )

might fix this, but I haven't been able to test this yet (no unit tests seem to address this at the moment).

Ideally, we'd want to add a new unit test to ensure these bounds are calculated correctly, but I'm not sure if I have the time to look into this further in the coming weeks.

nhniina commented 3 months ago

I tested this with my dataset and unfortunately it change didn't fix the upper bounds correctly.

manuelma commented 3 months ago

Nice catch! Sorry for the troubles. The issue here is I was assuming the replacement expression can only contain the same type of variable, like, if we are replacing a unit_flow, then the expression can only contain another unit_flow. But then we 'improved' it so the expression for a unit_flow can also contain a units_on and a units_started_up... So it is no longer straightforward to translate a bound for a replaced variable to a bound for the referred variable.

The quick fix would be to not use the replacement expression if the replaced variable has a bound, but that would also be a pitty. Will try to make it work in that case too, but if too difficult I'll just apply the quick fix.

manuelma commented 2 months ago

Fixed by https://github.com/spine-tools/SpineOpt.jl/pull/1063