Skip to content

Comments

Update examples and tutorials to not use retrofit agents#349

Merged
tsmbland merged 47 commits intodevelopfrom
documentation
Jul 2, 2024
Merged

Update examples and tutorials to not use retrofit agents#349
tsmbland merged 47 commits intodevelopfrom
documentation

Conversation

@tsmbland
Copy link
Collaborator

@tsmbland tsmbland commented Jun 19, 2024

Description

Sorry for the massive PR - this ended up being a bigger job than I expected. Most of the changes, however, are to results files.

The point of this PR is to change most of the example and tutorial models to use agents with a single 'new' share, rather than a pair of 'new' and 'retrofit' shares. This is a more likely scenario for basic users, and makes the tutorials simpler.

This PR also includes the changes made in #386 to increase the maximum_iterations parameter.

Main changes:

  • Change the example models to use standard_demand instead of new_and_retro, remove the new_to_retro interaction, remove the Agent2 retrofit agent share, and allocate all existing capacity to the Agent1 (new) agentshare
  • These changes have been propagated to the tutorial models, which are all based on the default and default_timeslice` models
  • Add a default_retro model to the examples, which is equivalent to the old default model
  • The only example model I haven't changed is the trade model, as I couldn't get it to work without the retrofit agents.
  • Change the default value of demand_share to standard_demand
  • Modify the add_agent parameter in the wizard module to work without retrofit agents
  • Update tests to work with the new models
  • Changes to the tutorial notebooks in line with the changes to the models. You can see the new version of the tutorials here.

It would probably be a good idea to have a tutorial using retrofit agents in the advanced guide, but I think that should be a separate issue.

Fixes #340
Fixes #350

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

I've just updated the inputs for some of the models so far (haven't regenerated the results), but does this look reasonable? @dalonsoa

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.

I think these changes look sensible, so hopefully they should result in feasible models. Have you tried to run them? Do they result in any error?

@tsmbland
Copy link
Collaborator Author

Yeah they all run and the results look similar to before, so I think it's all good

@codecov
Copy link

codecov bot commented Jul 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.29%. Comparing base (9456a3f) to head (9ee059d).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #349      +/-   ##
===========================================
- Coverage    71.37%   71.29%   -0.09%     
===========================================
  Files           44       44              
  Lines         5890     5901      +11     
  Branches      1162     1166       +4     
===========================================
+ Hits          4204     4207       +3     
- Misses        1365     1373       +8     
  Partials       321      321              

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

@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 2, 2024

I have no idea why the regressions tests are failing. @dalonsoa @alexdewar Can you try running tutorial 4.2 locally in this branch and let me know if you get any diffs?

@tsmbland tsmbland requested review from alexdewar and dalonsoa July 2, 2024 11:09
@tsmbland tsmbland marked this pull request as ready for review July 2, 2024 11:19
@dalonsoa
Copy link
Collaborator

dalonsoa commented Jul 2, 2024

This is failing also locally for me. I think it makes sense. The time framework in the settings is [2020, 2022, 2024, 2026, 2028, 2030, 2032, 2034, 2036, 2038, 2040] but in the Results folder that is use as the "correct" values there are also 2025, 2035, 2045 and 2050, which I guess should not be there. As the tests compares the "correct" output with the new output, it fails. You will need to remove the files that should not be there anymore.

image

@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 2, 2024

Makes sense! Thanks

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 only given this a quick once-over, but it seems sensible.

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.

Now that it passes, all looks good!

@tsmbland tsmbland merged commit 110ab54 into develop Jul 2, 2024
@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 3, 2024

I've just realised that changing the default demand share to "standard_demand" could be a breaking change for models where demand share isn't explicitly stated in the settings file. Should I change it back to "new_and_retro"? @dalonsoa @ahawkes

@alexdewar
Copy link
Collaborator

I guess we have to change it back if it's a breaking change 😞.

We could issue a warning if people don't specify the demand share though, saying something like "it's defaulting to new_and_retro for backwards compatibility reasons but you probably don't want to use this"

@tsmbland
Copy link
Collaborator Author

tsmbland commented Jul 3, 2024

Broader point - but how essential is it that we don't make breaking changes, since we know everyone that's using the software and can warn them of these sorts of things? (ignoring the technicalities of semantic versioning)

@alexdewar
Copy link
Collaborator

Good question... I'd personally still be inclined to print a warning because even if we know all the users, we don't have any guarantees that they're running the version of the code we asked them to and they might well update by accident! I guess it's up to @ahawkes though

@ahawkes
Copy link
Collaborator

ahawkes commented Jul 3, 2024 via email

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.

Change default value for demand_share to "standard_demand" Update examples/tutorials to NOT using retrofit agents

4 participants