Skip to content

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Jul 1, 2024

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_path format, 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. Adding errors="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_vars statements.

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.

  • New feature (non-breaking change which adds functionality)
  • Optimization (non-breaking, back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (whatever its nature)

Key checklist

  • All tests pass: $ python -m pytest
  • The documentation builds and looks OK: $ python -m sphinx -b html docs docs/build

Further checks

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@tsmbland tsmbland marked this pull request as ready for review July 2, 2024 08:55
@tsmbland tsmbland requested review from alexdewar and dalonsoa July 2, 2024 08:56
@codecov
Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.37%. Comparing base (0d54942) to head (a7367bb).

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.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@alexdewar alexdewar left a 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.

@dalonsoa
Copy link
Collaborator

dalonsoa commented Jul 2, 2024

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 month, day, hour, but that is just a convention. One could equally use season, week, hour.

@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 2, 2024

Yes that's a good idea. I'll give that a try in #381

@tsmbland tsmbland changed the base branch from develop to drop_timeslice July 2, 2024 14:15
@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 2, 2024

Opened #392 to deal with timeslice dropping. Just need this very small change as well to fix the issue

Base automatically changed from drop_timeslice to develop July 4, 2024 08:23
@tsmbland tsmbland enabled auto-merge July 4, 2024 08:24
@tsmbland tsmbland merged commit e74700c into develop Jul 4, 2024
@tsmbland tsmbland deleted the demand_path branch July 4, 2024 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

No longer compatible with consumption data in the demand_path format [BUG]

4 participants