Changed the default number of units based on is_candidate. #1036
Conversation
|
Nice work @Mastomaki - this makes a lot of sense - looks like all tests are passing and it can be merged. @manuelma does this look ok to you? Edit: once the conflicts are resolved I suppose |
| ) | ||
| end | ||
|
|
||
| # Change the default number of units so that it is zero when candidate units are present | ||
| # and otherwise 1. | ||
| _default_number_of_units(u) = is_candidate(unit=u) ? 0 : 1 |
There was a problem hiding this comment.
Should the 1 be number_of_units(...) instead?
There was a problem hiding this comment.
I now changed the code to reflect this and updated the unit test.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1036 +/- ##
=======================================
Coverage 86.82% 86.82%
=======================================
Files 143 143
Lines 4121 4121
=======================================
Hits 3578 3578
Misses 543 543 ☔ View full report in Codecov by Sentry. |
| + number_of_units(m; unit=u, stochastic_scenario=s, t=t) | ||
| # Change the default number of units so that it is zero when candidate units are present | ||
| # and otherwise 1. | ||
| + ifelse(is_candidate(unit=u), 0, number_of_units(m; unit=u, stochastic_scenario=s, t=t) ) |
There was a problem hiding this comment.
Doesn't this disallow having both number_of_units and candidate_units different than zero? It looks like if the user specifies candidate_units different than zero then any non zero value for number_of_units is just ignored? Or I am missing something?
There was a problem hiding this comment.
Indeed. Now I tried to correct it.
…mber_of_units because one commit was missing in local repo.
This one adds the same functionality for units as is already for storages and connections, namely overriding the default number of units.
However, it can be confusing to the user because as far as I see the overriding has not been mentioned in the documentation.