Align samplers and fix cost gradient

[NicolaCourtier]

Description

These changes isolate the interface between the optimiser/sampler and the cost. The new interface class becomes the only place where the following actions are performed:

1. transformation of the parameter values,
2. inversion of the cost value for min/maximisation, and
3. inversion and inverse transformation on any gradient.

The cost interface is tested in tests/unit/test_cost_interface.py. By paying off some technical debt, we can reduce the load for future debugging!

This PR includes bug fixes for:

• inverse transformation of the gradient for non-separable problems
• transformation of the parameter bounds for nonlinear transformations
• the derivative of the Gaussian logpdf
• the log_jacobian_det of the ScaledTransformation

as well as the corresponding unit tests.

Issue reference

Show fix

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.
• Examples: A change to existing or additional examples.
• 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 doctest

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]

Codecov Report

Attention: Patch coverage is 99.61977% with 1 line in your changes missing coverage. Please review.

Project coverage is 99.23%. Comparing base (828a8e3) to head (b2e472f).
Report is 48 commits behind head on develop.

Files with missing lines	Patch %	Lines
pybop/optimisers/_result.py	99.28%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #661      +/-   ##
===========================================
- Coverage    99.27%   99.23%   -0.05%     
===========================================
  Files           64       66       +2     
  Lines         4813     4816       +3     
===========================================
+ Hits          4778     4779       +1     
- Misses          35       37       +2     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[BradyPlanden]

Hi @NicolaCourtier, I have two functionality concerns with this PR as is, they are:

• Transformations can no longer be applied to the cost class directly. This complicates the ask-tell interface, and further limits the functionality of using PyBOP to construct costs and then optimise elsewhere. Was there specific reason for this change?
• Bounds now require a dictionary, and are not available as construction via a list-like object (this was purposefully added in Adds MAE and MSE cost classes #621). A simple list for rectangle-bounds is something I would like to keep in the codebase.

I think it's worth discussing the above before reviewing the remainder. Alternatively, if you split off the other changes I suspect we can merge those quite quickly. Thanks for the additions! :)

[martinjrobins]

thanks @NicolaCourtier :) Its a nice congregation of the transformation features into a single interface, which I'm in favour of and it simplifies the code and makes the transformation application easy to find. I have some comments on the general structure of this, but since this is blocking other things and fixes bugs, it might be better to just fix a few immediate things (which I've commented below), and then merge it in. Can deal with more structural things in subsequent PRs.

I find the structure of the classes a bit confusing, I wouldn't use inheritance for this but simply transform the cost itself to a "transformed cost" which acts like any regular cost function. You can see how we've implemented this in PINTs here: https://github.com/pints-team/pints/blob/015b1e8ada5937693ab7415d038f5c101f2a78d4/pints/_transformation.py#L1184. So any callable can be transformed into a different space by the application of a transformation. And you only have to do it once, then anything downstream (optimisers, plotting, other stuff executing the cost function) doesn't need to know about the transformation

I'd be keen to chat further about how costs should be structured, might be easier to chat face-to-face or via teams rather than a PR review tho!

[BradyPlanden]

Hi Nicola, thanks for the additions and fixes. A few comments to discuss, otherwise looks good.

[BradyPlanden]

Should this stay as a private attribute with a property? I don't believe we support users modifying it.

[NicolaCourtier]

I have updated the attribute minimising to invert_cost so that we can cope with optimiser minimising by default and samplers maximising. This property is applied during run (to transform the candidate values and to inverse transform them back for the log and result). So I believe it can be changed between runs.

[BradyPlanden]

I believe you mentioned there was a reason you had to pass self.cost instead of accessing it through the class. Can you bring me up to speed on that?

[NicolaCourtier]

To confirm, it is only this line that necessitates the cost being passed as an input - it sounds like this is something we can resolve in future.

[BradyPlanden]

This offset is need for at the Cuckoo & RandomSearch optimisers at moment. For robustness, the bounds are shifted by the smallest increment on the machine to ensure they are finite. We can probably update this in the corresponding optimisers, but at the moment this change breaks those two. For reference, this was added in #595.

[NicolaCourtier]

I have opened issue #671 to remind us to carry out a rigorous review of bounds.

[martinjrobins]

looks great, thanks @NicolaCourtier