EnergySystemsModellingLab / MUSE_OS

Welcome to the MUSE-OS repository
https://muse-os.readthedocs.io/en/latest/
GNU General Public License v3.0
22 stars 8 forks source link

Update tutorial notebook text #312

Closed tsmbland closed 1 week ago

tsmbland commented 1 month ago

Description

This PR makes a number of changes to the tutorial notebooks and underlying models. The tutorials are by no means finished, this is more just a step in the right direction, but this PR is getting quite large now (sorry), so I think it's worth reviewing and merging.

The main purpose of this PR was to update the text in the notebooks to make sure they have all the information needed to replicate the results. I have also made a few changes (simplifications) to the underlying models which will hopefully make it easier for users to follow the tutorials.

At some point we will want someone with domain knowledge to go through all of these and make sure they make sense, but I think this can wait until the remaining issues are completed.

Main things still to do:


Summary of changes:

General changes:

Section 4:

Section 8.1:

Section 8.2:

Section 8.4:

Section 8.5:

Section 8.6:

Section 8.7:


It's probably easiest to review by taking a glance at the documentation site for this branch (here), but bear in mind that tutorials 2 and 7 won't make much sense until the remaining issues are fixed

Fixes #311

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

Further checks

codecov[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 71.18%. Comparing base (7c8036c) to head (cb3bd87).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #312 +/- ## ======================================== Coverage 71.18% 71.18% ======================================== Files 44 44 Lines 5844 5844 Branches 1158 1158 ======================================== Hits 4160 4160 Misses 1363 1363 Partials 321 321 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tsmbland commented 2 weeks ago

Might be a good one for @HarmonicReflux to review (i.e. work through the new notebooks and make sure you're able to replicate the results)

tsmbland commented 2 weeks ago

Pesky regression tests failing on macos - not sure what to do about this...

alexdewar commented 1 week ago

The tests appear not to be running... Does anyone have any idea why?

dalonsoa commented 1 week ago

No idea. They were running yesterday, I think.

dalonsoa commented 1 week ago

I've just closed and re-open the PR. That has triggered the workflows again.

dalonsoa commented 1 week ago

And now, everything fails... :(

tsmbland commented 1 week ago

Numpy 2.0, yay!

dalonsoa commented 1 week ago

Here we go...

alexdewar commented 1 week ago

FYI There's a ruff rule to upgrade to numpy 2.0 (NPY201): https://numpy.org/devdocs/numpy_2_0_migration_guide.html

alexdewar commented 1 week ago

I've only just noticed that we're not pinning dependency versions... yikes.

How about for now we just change the numpy requirement to be numpy<2 so that we can finish this PR and #328? We can always fix the numpy thing later.

alexdewar commented 1 week ago

PS I tried the ruff rule and it seems it may not be the panacea I was hoping. It only identified one problem:

diff --git a/src/muse/quantities.py b/src/muse/quantities.py
index 604286d5..1fa4cad8 100644
--- a/src/muse/quantities.py
+++ b/src/muse/quantities.py
@@ -574,7 +574,7 @@ def supply_cost(
         else:
             data = data.groupby("region").sum(asset_dim)

-    total = data.production.where(np.abs(data.production) > 1e-15, np.infty).sum(
+    total = data.production.where(np.abs(data.production) > 1e-15, np.inf).sum(
         "timeslice"
     )
     return data.prices / total
tsmbland commented 1 week ago

Thanks @alexdewar for the good points! I've made the changes you suggested.

Yeah, I agree that I should have broken this up a bit more into multiple PRs - my bad. Hopefully future changes will be smaller.

I've pinned numpy to <2.0 (and opened #345), but I'm still having issues with some of the tests (unrelated to numpy 2.0). It's just that some of the results on macos are ever-so-slightly different to the expected results, but I don't know why this is or how to fix it (maybe we just need to change the tolerance?)

I've seen issues like this before and usually you can fix it by re-running the tests, but this seems particularly stubborn...

alexdewar commented 1 week ago

Yeah, I agree that I should have broken this up a bit more into multiple PRs - my bad. Hopefully future changes will be smaller.

Nw!

I've pinned numpy to <2.0 (and opened #345), but I'm still having issues with some of the tests (unrelated to numpy 2.0). It's just that some of the results on macos are ever-so-slightly different to the expected results, but I don't know why this is or how to fix it (maybe we just need to change the tolerance?)

I've seen issues like this before and usually you can fix it by re-running the tests, but this seems particularly stubborn...

I think increasing the tolerance might be the way we have to go. Unfortunately there are subtle differences in the way different processor manufacturers design their floating-point units and so you often get slightly different results on Intel/AMD/ARM. (Come to think of it, I've got an AMD processor here, so maybe I should try running the regression tests too...)

tsmbland commented 1 week ago

Fixed the failing tests by increasing rtol to 1e-4 (which I think is still small enough to be meaningful)