-
Notifications
You must be signed in to change notification settings - Fork 10
Enable consumption data in demand_path format
#384
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #384 +/- ##
========================================
Coverage 71.37% 71.37%
========================================
Files 44 44
Lines 5928 5928
Branches 1169 1169
========================================
Hits 4231 4231
Misses 1374 1374
Partials 323 323 ☔ View full report in Codecov by Sentry. |
alexdewar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
In terms of how we catch things like this in future, I agree that we probably don't want another example model, but it might still be useful to have @sharwaridixit's model be run as part of the regression tests, if it's using code paths that are otherwise untested. Would it be useful to have another set of models which are just used for those tests? Alternatively, I guess we could add a unit test or two.
|
I think that, at this point, we might want to recover @alexdewar idea of a helper function to drop these. Something like: def drop_timeslice(data):
if "timeslice" not in data.dims:
return data
return data.drop_vars([c for c in data.timeslice.indexes])This would also be independent on the name of the timeslice levels. Normally they are |
|
Yes that's a good idea. I'll give that a try in #381 |
|
Opened #392 to deal with timeslice dropping. Just need this very small change as well to fix the issue |
Description
This is a not very elegant way of getting Sharwari's model from #338 to work with the latest MUSE version. The problem was that she was specifying the demand data in a format that doesn't have timeslice information (the
demand_pathformat, see here), which is a valid scenario and something that worked before, but something that we accidentally broke with (I think) #289.Specifically, a couple of places in the code were complaining because it was trying to call
drop_vars(["timeslice", "month", "day", "hour"])on xarrays that don't have timeslice data. Addingerrors="ignore"means that it will try to drop these variables, and not raise a fuss if they already don't exist.See also #381 which is also related to these
drop_varsstatements.Ideally we'd have a test to make sure this doesn't break again, although I'm not sure the best way to do that without creating a new example model for the regressions tests, which seems a bit overkill. @dalonsoa - ideas?
Note also that Sharwari's model still fails due to #383 (although that one could be easily bypassed by removing the output)
Fixes #364
Type of change
Please add a line in the relevant section of
CHANGELOG.md to
document the change (include PR #) - note reverse order of PR #s.
Key checklist
$ python -m pytest$ python -m sphinx -b html docs docs/buildFurther checks