Fixes breaking changes from #3624

[BradyPlanden]

Description

This PR updates ParameterSet.update() and Solver.solve() so that #3624 is non-breaking.

Fixes #4071

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. If necessary, also add to the list of breaking changes.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

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 tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.54%. Comparing base (5f35628) to head (aeff677).
Report is 214 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4072   +/-   ##
========================================
  Coverage    99.54%   99.54%           
========================================
  Files          287      287           
  Lines        21744    21753    +9     
========================================
+ Hits         21646    21655    +9     
  Misses          98       98           

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

[kratman]

Out of curiosity, if we did the renaming of the parameter, where is the old name coming from in PyBaMM usage?

In #4071 it looks like you manually passed the old name into the function. Is there another tool in the PyBaMM ecosystem that is still using the old name?

[BradyPlanden]

Fair question. The previous name would be used in any script/notebook/package built before v24.5, and in those cases, if ParameterSet.update() or Solver.solve(inputs={old_name:value}) is called, the change would be breaking. #3624 both renames the parameter and does the conversion in the FuzzyDict, but misses the above methods. This PR fixes that.

[kratman]

I know we have deprecation warnings for the parameter name changes, I think if anything we should update them to catch it better. Putting it is the base solver seems like the wrong approach. Once we are out of the deprecation window then we would expect to be able to remove the code checking for it.

[BradyPlanden]

Putting it is the base solver seems like the wrong approach. Once we are out of the deprecation window then we would expect to be able to remove the code checking for it.

I agree it's not a very clean solution, but given the inputs arg is a user defined dictionary and directly passed from user-level I don't believe we can catch it before that point, unless you see an alternative? #3624 is marked non-breaking, so the aim of this PR is to ensure that is the case (this would be helpful for dependant packages like PyBOP).

How do we track removal of depreciated code at the moment? Perhaps adding depreciation warnings around the additions in this PR and adding them to a log for removal after two release would ensure they get removed when ready.

[brosaplanella]

We discussed about deprecation warnings in #2028 a while ago, but not much progress has been made.

[valentinsulzer]

Can you share the scripts that are broken on develop and work with this fix?

[kratman]

Summary of my comments from the meeting: "Can the check for the old parameter name be done only in the update function and add a warning"

[BradyPlanden]

Can you share the scripts that are broken on develop and work with this fix?

Sure @valentinsulzer, any of these with the old parameter name used in parameter_values.update() or the inputs object for solve() will break in develop and be fixed by this PR. It's any time ParameterSet.update({name:value}) or solver.solve(input={name:value}) uses the previous parameter name, both of which are used throughout the examples.

"Can the check for the old parameter name be done only in the update function and add a warning"

Yep, I'll sort this and drop a line once it's ready for review.

[valentinsulzer]

Nice preview of how painful it is to rename a parameter, for you guys leading the parameter refactor effort

[brosaplanella]

Looks good to me, thanks! It would be good to get a second review, just for a second opinion on how to handle the warnings. BTW, could you add a line to the CHANGELOG?

[BradyPlanden]

There are a few tests that are failing periodically at the moment, but it seems to be across the repo. For coverage, the lines in question are being missed periodically, but not as a function of this PR. This should be good for the final review now.