Improvements for StrategicScenario

[JulStraus]

The existing version for StrategicScenario was existing, but not necessarily in the same format as the other iterators. Specifically, the following problems were identified:

• we did not provide all iteration utilities as we do for the other iterators,

• the different scenarios were not equal when creating the iterator:

  a = strategic_scenarios(ts)
  b = strategic_scenarios(ts)
  first(a) == first(b)  # returned false

• we did not have a wrapper for single strategic scenarios, and

• we did not support yet the function `strat_periods(strategic_scenarios(ts)).

Specifically, the issue with the equality was a problem as it hindered us to use a strategic scenario as index. The problem was the utilization of the Vector which is mutable, even if the type in which the Vector is a field of is immutable. After checking the documentation, I realized it is beneficial to have only immutable field entries.

Note

Changes in naming and the structure of the types should be non-breaking as we did not export them before.

[trulsf]

I like your tautology 'The existing version for StrategicScenario was existing' ;)

Overall I find this fix solid, but it seems to be some inconsistency in how the wrapping of another time structure works compared to calling strategic_scenarios on a TwoLevelTree.

The following example illustrates the problem

week = SimpleTimes(7,1)
two_level_tree = regular_tree(3, [2, 3], week)
scen1 = first(strategic_scenarios(two_level_tree))
sps1 = collect(scen1)

two_level = TwoLevel(3, week)
scen2 = first(strategic_scenarios(two_level)
sps2 = collect(scen2)

Here sps1 will collect the strategic nodes of the scenario while sps2 will have all time periods. Iterating the strategic periods leads to similar behavior in both cases

sps1 = collect(strat_periods(scen1))
sps2 = collect(strat_periods(scen2))

I am not sure which pattern I prefer, as long as the same pattern produces the same behavior. In the documentation we have the example

for sc in strategic_scenarios(two_level_tree)
    @constraint(m, sum(invest[sp] for sp in sc) <= 1)
end

that may have to be altered if we go for the second pattern

for sc in strategic_scenarios(two_level_tree)
    @constraint(m, sum(invest[sp] for sp in strat_periods(sc)) <= 1)
end

[JulStraus]

Overall I find this fix solid, but it seems to be some inconsistency in how the wrapping of another time structure works compared to calling strategic_scenarios on a TwoLevelTree.

Good that you spotted it. We spent quite some time to unify it with one example being PR #9 so we should not introduce new iterators that result in a weird behaviour again. I would personally argue, based on the philosophy of the subtypes, that iterating through a StrategicScenario should be equivalent to iterating through a TwoLevel. Otherwise, we can run into issues in the long term.

Important

If I see it correctly, we also had this problem beforehand? So in this case, changing the behaviour would result in a breaking change

[trulsf]

I agree, I also find it natural that a StrategicScenario iterates as a TwoLevel.

While strictly breaking, I think it is a low likelihood that anyone has used this functionality. I guess, making a breaking release is most hassle on EMX side.

[JulStraus]

I agree, I also find it natural that a StrategicScenario iterates as a TwoLevel.

While strictly breaking, I think it is a low likelihood that anyone has used this functionality. I guess, making a breaking release is most hassle on EMX side.

It would be a bit of a hazzle (we would need to update all packages with a new version) but we are also looking in some breaking parts anyhow. I will update it with the correct iteration functionality, but do not change anything regarding version numbers yet.

[JulStraus]

I created now a new commit which is changing the behaviour of the iterator. Given that the previous version was inconsistent in itself, I would argue that we can see the new change as a bug fix. This would hence imply that we do not see it as a breaking change.

[trulsf]

If we do this as a bugfix, can you update version number and I will trigger a new release.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 98.30508% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/strat_scenarios/strat_scenarios.jl	98.21%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/op_scenarios/strat_periods.jl	69.30% <ø> (-4.20%)	⬇️
src/op_scenarios/tree_periods.jl	69.23% <100.00%> (-1.44%)	⬇️
src/representative/tree_periods.jl	73.07% <100.00%> (-2.79%)	⬇️
src/strat_scenarios/core_types.jl	98.76% <ø> (+14.55%)	⬆️
src/strat_scenarios/tree_periods.jl	95.65% <100.00%> (+11.65%)	⬆️
src/strat_scenarios/strat_scenarios.jl	98.21% <98.21%> (ø)

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.