Enforce minimum service factor limits (take 2) #328
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #328 +/- ##
===========================================
+ Coverage 71.24% 71.39% +0.15%
===========================================
Files 44 44
Lines 5870 5887 +17
Branches 1158 1162 +4
===========================================
+ Hits 4182 4203 +21
+ Misses 1366 1364 -2
+ Partials 322 320 -2 ☔ View full report in Codecov by Sentry. |
dalonsoa
left a comment
dalonsoa
left a comment
There was a problem hiding this comment.
The changes look ok, but I've a couple of comments/concerns, and a request.
src/muse/readers/csv.py
Outdated
|
|
||
| data = data.apply(lambda x: pd.to_numeric(x, errors="ignore")) | ||
| data = check_utilization_not_all_zero(data, filename) | ||
| check_utilization_not_all_zero(data, filename) |
There was a problem hiding this comment.
in principle, the technodata_timeslices file also has an utilization_factor column that will need validating. See https://github.com/EnergySystemsModellingLab/MUSE_OS/blob/enforce_minimum_service_factor_limits/src/muse/data/example/default_timeslice/technodata/power/TechnodataTimeslices.csv
| "minimum_service_factors", | ||
| permutations((np.linspace(0, 1, 6), [0] * 6)), | ||
| ) | ||
| def test_minimum_service_factor(tmpdir, minimum_service_factor, process_name): | ||
| def test_minimum_service_factor(tmpdir, minimum_service_factors): | ||
| import pandas as pd | ||
| from muse import examples | ||
| from muse.mca import MCA | ||
|
|
||
| sector = "power" | ||
| processes = ("gasCCGT", "windturbine") |
There was a problem hiding this comment.
Concrete values of the service factor aside, this does not check the same scenarios as before. Before there were 4 tests being launched, and now they are only 2. Not that the previous 4 cases were necessary, but just flagging this.
There was a problem hiding this comment.
There should still be 4... permutations() returns the inputs in 2 different orders
tsmbland
left a comment
tsmbland
left a comment
There was a problem hiding this comment.
Looks good, although I think we also need to have a similar check for MinimumServiceFactor in the read_technodata_timeslices function. While we're at it, it would also be good to have a similar check for UtilizationFactor (i.e. it needs to be between 0 and 1). And we could also check that UtilizationFactor is always >= MinimumServiceFactor.
Also, it might be nice to have a some tests with invalid inputs to make sure these checks fail when they should.
|
All good points @tsmbland! |
alexdewar
force-pushed
the
enforce_minimum_service_factor_limits
branch
3 times, most recently
from
a760cab to
4d1558b
Compare
|
Ok, I think I've made all the changes that were suggested and I've added a bunch more tests. I'm glad I had a more thorough look at this because it turns out my original fix didn't actually work 😟 |
dalonsoa
left a comment
dalonsoa
left a comment
There was a problem hiding this comment.
Looks good! Way more comprehensive validation than before.
|
But tests are failing... |
|
Sorry 😞. I pushed this at the end of the day and figured it was all fine (the tests I was working on were passing!). I'll take another look. Hopefully it's just something trivial! |
|
Converting to draft until I fix the test failures (see also #362) |
This reverts commit 320738c.
…um_service_factors
alexdewar
force-pushed
the
enforce_minimum_service_factor_limits
branch
from
310af65 to
b444f68
Compare
| @patch("muse.readers.csv._check_utilization_not_all_zero") | ||
| @patch("muse.readers.csv._check_utilization_not_below_minimum") | ||
| @patch("muse.readers.csv._check_minimum_service_factors_in_range") | ||
| def test_check_utilization_and_minimum_service_factors_fail_missing_utilization(*mocks): |
There was a problem hiding this comment.
I don't think you need the mocks here. Otherwise, everything looks good!
Let's try this again. It turns out the fix for my previous mistake was really trivial (5fb3ff5). Sorry about all the noise!
Fixes #320.