Skip to content

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Jul 1, 2024

Description

After #392, there are still a couple of places in the code that referenced "month", "day" and "hour" by name, which could cause problems if the level names were changed from the default (which is possible to in the settings file). This started off as a PR to remove all of these hardcoded references, but this ended up having a few other implications, so this PR now addresses a few things.

Specific changes:

scipy_match_demand in investments.py. This was easy to fix as it was just dropping the "month", "day" and "hour" columns needlessly, so I just removed this.

sector_supply and sector_consumption in mca.py which prepare a couple of output files. I found a workaround to do a similar thing without referring to "month", "day" and "hour" by name, and in the process slightly modified the output files (for the better, I think).

This second change then allowed test_minimum_service to start actually testing something (see #382), but the test was failing because it was testing the wrong thing. So I fixed the test so it's now testing the right thing, although the test is still failing, presumably because the supply data that it's using is wrong (see #321, #335 and #368). So not sure what to do now. @dalonsoa If we're planning on fixing the broken supply output, are you happy for me to mark this test as xfail for now so we can merge this?

Fixes #363
Fixes #382

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 changed the base branch from develop to demand_path July 2, 2024 15:31
@tsmbland tsmbland changed the title Stop hardcoding timeslice level names Remove any hardcoded reference to "month", "day" and "hour" Jul 3, 2024
@tsmbland tsmbland marked this pull request as ready for review July 3, 2024 12:49
@tsmbland tsmbland requested review from alexdewar and dalonsoa July 3, 2024 12:49
Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look totally sensible so I'm happy to move on with this one and marking the test as xfail if you're sure that the test is failing because it is not correctly setup and not because the changes to the functions you have made have broken the execution pipeline.

So we don't forget, also open an issue about it, assign it to me and add it to this iteration.

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.

Seems fine to me.

data_sector.append(a[a["consumption"] != 0])

if len(data_sector) > 0:
output = pd.concat([u for u in data_sector], sort=True).reset_index()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be a generator expression?

@codecov
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 71.36%. Comparing base (e74700c) to head (e830cd3).

Files Patch % Lines
src/muse/outputs/mca.py 68.75% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #381      +/-   ##
===========================================
- Coverage    71.37%   71.36%   -0.01%     
===========================================
  Files           44       44              
  Lines         5928     5916      -12     
  Branches      1169     1162       -7     
===========================================
- Hits          4231     4222       -9     
+ Misses        1374     1371       -3     
  Partials       323      323              

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

@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 3, 2024

@dalonsoa I've marked the test as xfail. We already have #335 which should fix the test when it's sorted, but I'm not sure what we've doing about that one.

Base automatically changed from demand_path to develop July 4, 2024 08:38
@tsmbland tsmbland enabled auto-merge July 4, 2024 10:55
@tsmbland tsmbland merged commit 06d35af into develop Jul 4, 2024
@tsmbland tsmbland deleted the timeslice_names branch July 4, 2024 10:59
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.

test_minimum_service_factor is not actually testing anything Timeslice level names are no longer configurable [BUG]

4 participants