Skip to content

Comments

Update tutorial notebook text#312

Merged
tsmbland merged 38 commits intodevelopfrom
documentation
Jun 19, 2024
Merged

Update tutorial notebook text#312
tsmbland merged 38 commits intodevelopfrom
documentation

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented May 17, 2024

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:

  • fixing links
  • fixing minor typos (e.g. technodata.csv -> Technodata.csv, projections.csv -> Projections.csv)
  • be more specific about the starting point for each tutorial
  • changes to tables so they actually show the right numbers

Section 4:

  • shrinking plots so they can be viewed side-by-side

Section 8.1:

  • simplify model to be more in line with default
  • add note about excluded_commodities in settings.toml
  • show data for all sectors

Section 8.2:

  • switch order of models to have the simpler scenario first
  • the population wasn't being split properly between both agents, this is now fixed
  • show data for all sectors

Section 8.4:

  • simplify the consumption profile
  • a few changes to growth/capacity parameters
  • add note about forecast parameter in settings.toml

Section 8.5:

  • rename model back to what it originally was (fixes some links which were broken)
  • a few changes to growth/capacity parameters

Section 8.6:

  • plot sectors side-by-side

Section 8.7:

  • clarify the starting point as the default_timeslices model. This is a change compared to before, where default was the starting point, and should make it much easier for the user
  • set MinimumServiceFactor to zero by default (this will be unecessary once Change MinimumServiceFactor for default_timeslices model #334 is done)

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.

  • 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 linked an issue May 17, 2024 that may be closed by this pull request
Base automatically changed from generate_tutorial_models to develop May 22, 2024 10:24
@tsmbland tsmbland marked this pull request as ready for review June 14, 2024 13:38
@tsmbland
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@alexdewar
Copy link
Collaborator

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

@dalonsoa
Copy link
Collaborator

No idea. They were running yesterday, I think.

@dalonsoa dalonsoa closed this Jun 18, 2024
@dalonsoa dalonsoa reopened this Jun 18, 2024
@dalonsoa
Copy link
Collaborator

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

@dalonsoa
Copy link
Collaborator

And now, everything fails... :(

@tsmbland
Copy link
Collaborator Author

Numpy 2.0, yay!

@dalonsoa
Copy link
Collaborator

Here we go...

@alexdewar
Copy link
Collaborator

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

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.

I've made various small suggestions about parts of the text. On the whole, it looks great though!

I think this PR could have been broken up a bit more though. Personally I think it would be fine to do one PR per tutorial -- and it makes it a bit easier to digest.

@@ -61,7 +61,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"We will now save this file and run the new simulation model using the following command in Anaconda prompt:\n",
"We will now save this file and run the new simulation model using the following command-line prompt:\n",
"\n",
" python -m muse settings.toml\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering whether we should be telling people to run muse settings.toml instead these days. It looks like there are various places where we suggest running python -m muse... worth opening an issue for? What do you think @dalonsoa?

@alexdewar
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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

@alexdewar alexdewar mentioned this pull request Jun 18, 2024
8 tasks
@tsmbland tsmbland requested a review from alexdewar June 18, 2024 22:21
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!

@tsmbland tsmbland merged commit b47973b into develop Jun 19, 2024
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.

Update notebook text to match updated models

3 participants