Restructure optimisers to enable passing of kwargs #255

NicolaCourtier · 2024-03-23T11:38:33Z

Description

Enable valid keyword arguments to be passed through the set-up of an Optimisation class and restructure the optimiser classes so that all PINTS and SciPy optimisers are subclasses of the same BaseOptimiser.

Issue reference

Fixes #236.

Review

Before you mark your PR as ready for review, please ensure that you've considered the following:

Updated the CHANGELOG.md in reverse chronological order (newest at the top) with a concise description of the changes, including the PR number.
Noted any breaking changes, including details on how it might impact existing functionality.

Type of change

New Feature: A non-breaking change that adds new functionality.
Optimization: A code change that improves performance.
Bug Fix: A non-breaking change that addresses an issue.
Documentation: Updates to documentation or new documentation for new features.
Refactoring: Non-functional changes that improve the codebase.
Style: Non-functional changes related to code style (formatting, naming, etc).
Testing: Additional tests to improve coverage or confirm functionality.
Other: (Insert description of change)

Key checklist:

No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
All unit tests pass: $ nox -s tests
The documentation builds: $ nox -s docs

You can run integration tests, unit tests, and doctests together at once, using $ nox -s quick.

Further checks:

Code is well-commented, especially in complex or unclear areas.
Added tests that prove my fix is effective or that my feature works.
Checked that coverage remains or improves, and added tests if necessary to maintain or increase coverage.

Thank you for contributing to our project! Your efforts help us to deliver great software.

codecov · 2024-03-23T11:53:18Z

Codecov Report

Attention: Patch coverage is 98.31461% with 6 lines in your changes missing coverage. Please review.

Project coverage is 96.93%. Comparing base (0d0e214) to head (6b37c3f).
Report is 444 commits behind head on develop.

Files	Patch %	Lines
pybop/optimisers/scipy_optimisers.py	95.45%	4 Missing ⚠️
pybop/optimisers/base_optimiser.py	97.14%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #255      +/-   ##
===========================================
+ Coverage    95.81%   96.93%   +1.12%     
===========================================
  Files           39       41       +2     
  Lines         2101     2222     +121     
===========================================
+ Hits          2013     2154     +141     
+ Misses          88       68      -20

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

BradyPlanden

Well done @NicolaCourtier - thanks for the addition!

I've added a few comments for you to look at. One addition is to update optimiser.needs_sensitivities depending on self.options["method"]. If not added in this PR, we should open an issue to add gradient support for the scipy optimisers.

Also, we should probably align the name of the bounds variable between the pints and the scipy optimisers. This can also be done in a separate PR.

pybop/_optimisation.py

pybop/optimisers/scipy_optimisers.py

pybop/_optimisation.py

pybop/optimisers/scipy_optimisers.py

NicolaCourtier · 2024-04-16T17:56:37Z

This PR now includes changes to the PINTS interface as well as SciPy and introduces a true BaseOptimiser. Addressing @BradyPlanden's points above:

Added gradient support for the SciPy optimisers, via the jac option. Note that SciPy optimisers do not need the needs_sensitivities property.
The same name bounds is used for all optimisers, although more could be done to align the bounds type.
Added update_options to the BasePintsOptimiser to unpack the optimiser_kwargs and try to apply them to the corresponding optimiser variables, with a final catch for unexpected keywords.

Note that this update nests the PINTS optimiser within the optimiser property of an Optimisation object... so I'm open to discussion around these changes.

BradyPlanden

I thought I would add a review on this even though it's not marked ready. It's a great step for the optimisers. There are a few areas I think we can improve, but thanks for the addition @NicolaCourtier!

examples/scripts/spm_NelderMead.py

examples/scripts/spm_adam.py

examples/scripts/spm_descent.py

pybop/_optimisation.py

pybop/optimisers/base_optimiser.py

pybop/optimisers/pints_optimisers.py

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

This reverts commit aa73bff.

BradyPlanden

LGTM, thanks for the additions @NicolaCourtier!

* Splits Optimisation -> BaseOptimiser/Optimisation, enables two optimisation APIs, updts where required, moves _optimisation to optimisers/ * increase coverage * Pass optimiser_kwargs though run() * updt examples * Converts DefaultOptimiser -> Optimisation * split Optimisation and BaseOptimsier classes, loosen standalone cost unit test * add incorrect attr test * fix: updt changelog entry, optimsation_interface notebook, review suggestions * fix: updt notebook state * Updt assertions, optimisation object name --------- Co-authored-by: NicolaCourtier <45851982+NicolaCourtier@users.noreply.github.com>

* Enable passing of kwargs to SciPy * Update checks on bounds * Add update_options and error for Pints * Rename opt to optim * Remove stray comments * Add BaseSciPyOptimiser and BasePintsOptimiser * Align optimiser option setting * Update notebooks with kwargs * Update scripts with kwargs * Update notebooks * Align optimisers with Optimisation as base class * Update stopping criteria in spm_NelderMead.py Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Update stopping criteria in spm_adam.py Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Update sigma0 in spm_descent.py Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Update GradientDescent * Change update to set and check pints_method * Update test_optimisation_options * Update notebooks * Update set learning rate * Pop threshold * Fix bug in model.simulate * Update notebooks * Update test_models.py * Store SciPy result * Update x0 input and add tests * Update bounds to avoid x0 outside * Re-initialise pints_method on certain options * Update x0_new test * Update test_optimisation.py * Create initialise_method for PINTS optimisers * Align optimisation result * Update checks on bounds * Apply suggestions Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Add standalone optimiser * Simplify optimiser set-up and align _minimising * Update option setting in notebooks * Take abs of cost0 * Implement suggestions from Brady * Update tests and base option setting * Update test_invalid_cost * Increase coverage * Sort out notebook changes * Reset scale parameter * Move settings into arguments * Update comments * Update optimiser call * Move check on jac * Add assertions * Add maxiter to test * Add assertion * Update to lambda functions Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Update comment Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Update to list comprehension Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com> * Formatting * Revert "Update to lambda functions" This reverts commit aa73bff. * Move minimising out of costs * Update description * Updates to #236 to avoid breaking change to `pybop.Optimisation` (#309) * Splits Optimisation -> BaseOptimiser/Optimisation, enables two optimisation APIs, updts where required, moves _optimisation to optimisers/ * increase coverage * Pass optimiser_kwargs though run() * updt examples * Converts DefaultOptimiser -> Optimisation * split Optimisation and BaseOptimsier classes, loosen standalone cost unit test * add incorrect attr test * fix: updt changelog entry, optimsation_interface notebook, review suggestions * fix: updt notebook state * Updt assertions, optimisation object name --------- Co-authored-by: NicolaCourtier <45851982+NicolaCourtier@users.noreply.github.com> * Rename method to pints_optimiser * Rename base_optimiser to pints_base_optimiser * Rename _optimisation to base_optimiser --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

Enable passing of kwargs to SciPy

8aef4f9

NicolaCourtier and others added 4 commits March 23, 2024 18:05

Update checks on bounds

5e1843d

style: pre-commit fixes

6513e9c

Change assert False to not

ffc3a4e

Add update_options and error for Pints

c56d9e6

NicolaCourtier requested a review from BradyPlanden March 25, 2024 09:46

NicolaCourtier added 2 commits March 27, 2024 20:24

Merge branch 'develop' into 236-scipy-kwargs

0a53eb3

Rename opt to optim

22ab3b1

BradyPlanden reviewed Mar 28, 2024

View reviewed changes

pybop/_optimisation.py Outdated Show resolved Hide resolved

pybop/optimisers/scipy_optimisers.py Outdated Show resolved Hide resolved

pybop/_optimisation.py Outdated Show resolved Hide resolved

pybop/optimisers/scipy_optimisers.py Outdated Show resolved Hide resolved

NicolaCourtier and others added 9 commits April 9, 2024 11:12

Merge branch 'develop' into 236-scipy-kwargs

0169420

Remove stray comments

4a1f0e0

Merge branch 'develop' into 236-scipy-kwargs

833ecfe

Add BaseSciPyOptimiser and BasePintsOptimiser

129b999

Merge branch 'develop' into 236-scipy-kwargs

762c714

style: pre-commit fixes

d43b101

Align optimiser option setting

710d49e

Update notebooks with kwargs

00d2815

Update scripts with kwargs

3d7d9b8

BradyPlanden reviewed Apr 19, 2024

View reviewed changes

NicolaCourtier and others added 9 commits April 22, 2024 18:52

Merge branch 'develop' into 236-scipy-kwargs

7e098e7

Complete merge

d91e727

style: pre-commit fixes

9125b6a

Update notebooks

ea8e9a0

Merge branch 'develop' into 236-scipy-kwargs

b48e6d7

Align optimisers with Optimisation as base class

a8e60aa

Update stopping criteria in spm_NelderMead.py

a496ae2

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

Update stopping criteria in spm_adam.py

ddf2afa

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

Update sigma0 in spm_descent.py

d4dd9d2

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

NicolaCourtier and others added 17 commits May 17, 2024 16:14

Reset scale parameter

4be3103

Move settings into arguments

1419111

Update comments

4237ad5

Update optimiser call

4991a37

Move check on jac

ec027a5

Add assertions

40c91ee

Add maxiter to test

b663396

Add assertion

e2ae1dd

Update to lambda functions

aa73bff

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

Update comment

9fa1155

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

Update to list comprehension

dd6446b

Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>

Formatting

dc04f47

Revert "Update to lambda functions"

da2b742

This reverts commit aa73bff.

Move minimising out of costs

1627754

Improve conditioning of StandaloneCost

2c61500

Reset StandaloneCost

658181e

Update description

afaf69e

NicolaCourtier requested a review from BradyPlanden May 21, 2024 16:42

BradyPlanden approved these changes May 22, 2024

View reviewed changes

NicolaCourtier changed the title ~~Enable passing of kwargs to optimisers~~ Restructure optimisers to enable passing of kwargs May 22, 2024

BradyPlanden and others added 6 commits May 22, 2024 16:45

Rename method back to optimiser

f1fc913

Update comments

2f3690b

Change optimiser to pints_optimiser

55abec8

Rename base_optimiser to pints_base_optimiser

8f73469

Rename _optimisation to base_optimiser

6b37c3f

NicolaCourtier merged commit 9e0058f into develop May 22, 2024
29 checks passed

NicolaCourtier deleted the 236-scipy-kwargs branch May 22, 2024 20:32

BradyPlanden mentioned this pull request May 23, 2024

[Bug]: Update benchmarks post #255 #333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure optimisers to enable passing of kwargs #255

Restructure optimisers to enable passing of kwargs #255

NicolaCourtier commented Mar 23, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading

BradyPlanden left a comment

NicolaCourtier commented Apr 16, 2024

BradyPlanden left a comment

BradyPlanden left a comment

Restructure optimisers to enable passing of kwargs #255

Restructure optimisers to enable passing of kwargs #255

Conversation

NicolaCourtier commented Mar 23, 2024 • edited Loading

Description

Issue reference

Review

Type of change

Key checklist:

Further checks:

codecov bot commented Mar 23, 2024 • edited Loading

Codecov Report

BradyPlanden left a comment

Choose a reason for hiding this comment

NicolaCourtier commented Apr 16, 2024

BradyPlanden left a comment

Choose a reason for hiding this comment

BradyPlanden left a comment

Choose a reason for hiding this comment

NicolaCourtier commented Mar 23, 2024 •

edited

Loading

codecov bot commented Mar 23, 2024 •

edited

Loading