Update UC and ramping constraints with accumulated units

[datejada]

Pull request details

Describe the changes made in this pull request

Generalization of the accumulated units for all assets (including the ones that are not investable). This generalization simplifies the code in the capacity, unit commitment, and ramping constraints.

List of related issues or pull requests

Closes #790

Collaboration confirmation

As a contributor I confirm

• I read and followed the instructions in README.dev.md
• [NA] The documentation is up to date with the changes introduced in this Pull Request (or NA)
• Tests are passing
• Lint is passing

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (0fe4d8f) to head (053daaf).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #804   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines          964       963    -1     
=========================================
- Hits           964       963    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[datejada]

@abelsiqueira I am a bit puzzled by the result of the Codecov because:

1. locally it is 100%
2. If that line is missed, then the next ones would have been missed too

Any ideas or suggestions?

[abelsiqueira]

It's the same issue we had in the other PR. We moved the if-else to the outside (a few lines above).

[datejada]

It's the same issue we had in the other PR. We moved the if-else to the outside (a few lines above).

Can we get rid of the lookup during the refactoring? If the answer is yes, I can continue my changes with that strategy for now. But, if the answer is no, then I am willing to sacrifice the Codecov in favour of readability

[abelsiqueira]

My earlier comment was to transform this into the following where each condition has its own expression.

Suggested change

	y ∈ Y,
	a ∈ decommissionable_assets_using_simple_method∪decommissionable_assets_using_compact_method,
	],
	accumulated_units[y ∈ Y, a ∈ A],
	if a in decommissionable_assets_using_simple_method
	accumulated_units_simple_method[y, a]
	else
	elseif a in decommissionable_assets_using_compact_method
	sum(
	accumulated_units_compact_method[accumulated_set_using_compact_method_lookup[(
	a,
	y,
	v,
	)]] for v in V_all if (a, y, v) in accumulated_set_using_compact_method
	)
	else
	sum(values(graph[a].initial_units[y]))
	end
	)
	accumulated_units = model[:accumulated_units] = [
	if a in decommissionable_assets_using_simple_method
	@expression(model,
	accumulated_units_simple_method[y, a]
	)
	elseif a in decommissionable_assets_using_compact_method
	@expression(model,
	sum(
	accumulated_units_compact_method[accumulated_set_using_compact_method_lookup[(
	a,
	y,
	v,
	)]] for v in V_all if (a, y, v) in accumulated_set_using_compact_method
	)
	)
	else
	@expression(model,
	sum(values(graph[a].initial_units[y]))
	)
	end
	for y ∈ Y, a ∈ A
	]

[abelsiqueira]

I made a comment in the code trying to clarify what I meant

[datejada]

I made a comment in the code trying to clarify what I meant

Thanks, I got that, but then to access it I need a lookup, making the code less readable only to make the codecov happy :/

I mean, with that strategy, I can't any longer access the parameter accumulated_units[row.year, row.asset] but accumulated_units[accumulated_units_lookup[(row.year, row.asset)]] and it looks odd, only because the online codecov does not get that line...

[datejada]

@abelsiqueira commit 053daaf has the suggested changes. Let's continue with the lookup, but I will add to the refactor issue #701 this check to discuss it later

[datejada]

And for some reason, the Codecov is not running again; haha it doesn't like that I complained about it

[abelsiqueira]

Thanks for clarifying. I'm fine with decreasing codecov's requirement. There are settings that we can set to say ">90%" means passing, or something. We should discuss it later.

[datejada]

@abelsiqueira I am unsure why the codecov is not running with the updates. I tried to trigger it in #805, but it didn't work either :/

So, here is the local coverage report, if that helps

[abelsiqueira]

I did nothing, I think codecov was just tired. Nobody complained anywhere I searched, so no way to know for sure.

[datejada]

Great! So it is ready for review again 😊 @abelsiqueira

[abelsiqueira]

Thanks, LGTM

[gnawin]

@datejada @abelsiqueira I agree that the code now is less readable to make codecov happy. We did the same for the compact method. If we decrease the codecov requirement, lots of places can be cleaner.