Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove python-idaklu #4326

Merged
merged 11 commits into from
Aug 27, 2024
Merged

Conversation

pipliggins
Copy link
Contributor

@pipliggins pipliggins commented Aug 8, 2024

Description

Removes the legacy python-idaklu solver which calls IDAKLU but returns back to python for certain functions. With recent improvements to IDAKLU this can now be removed.

Reduces the number of solvers we need to maintain.

Fixes #4223

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.

  • Optimization (back-end change that speeds up the code)

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

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.42%. Comparing base (8057c16) to head (a377567).
Report is 202 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4326      +/-   ##
===========================================
- Coverage    99.46%   99.42%   -0.04%     
===========================================
  Files          289      289              
  Lines        22200    22156      -44     
===========================================
- Hits         22081    22029      -52     
- Misses         119      127       +8     

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


🚨 Try these New Features:

@MarcBerliner MarcBerliner mentioned this pull request Aug 20, 2024
6 tasks
@kratman
Copy link
Contributor

kratman commented Aug 22, 2024

@pipliggins What still needs to be done for this PR?

@pipliggins
Copy link
Contributor Author

@kratman I think it's pretty much ready to go. The only query (which I realise now I hadn't actually voiced anywhere, whoops) is what to do about some indirect changes flagged by codecov which leave several lines of code untested after removing the solver.
It's not clear to me that those lines are specific to the old IDAKLU solver, in fact I'm pretty sure at least some of them shouldn't be, but I've found it hard to work out why they aren't covered by other tests, or how to reach those lines by adding new ones. (@martinjrobins any thoughts as well?)

@kratman
Copy link
Contributor

kratman commented Aug 22, 2024

The only query (which I realise now I hadn't actually voiced anywhere, whoops) is what to do about some indirect changes flagged by codecov which leave several lines of code untested after removing the solver. It's not clear to me that those lines are specific to the old IDAKLU solver, in fact I'm pretty sure at least some of them shouldn't be, but I've found it hard to work out why they aren't covered by other tests, or how to reach those lines by adding new ones

I think a lot of the test coverage is actually indirect. Some tests are only run with specific parameter sets or certain modes might only be run in a test for something unrelated. Ultimately as large functions get broken up into smaller once, this should get better.

Personally a loss of a little coverage does not bother me, but I do wonder if some of the lost coverage could be from logic that was catching rare edge cases.

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Just minor suggestions. The coverage changes seem fairly small, but @martinjrobins should take a look as well

src/pybamm/models/base_model.py Show resolved Hide resolved
src/pybamm/solvers/idaklu_solver.py Outdated Show resolved Hide resolved
src/pybamm/solvers/idaklu_solver.py Outdated Show resolved Hide resolved
src/pybamm/solvers/idaklu_solver.py Outdated Show resolved Hide resolved
src/pybamm/solvers/idaklu_solver.py Outdated Show resolved Hide resolved
src/pybamm/solvers/idaklu_solver.py Outdated Show resolved Hide resolved
Copy link
Contributor

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

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

Thanks @pipliggins, looking forward to no longer supporting this :) The codecov errors reminded me: now the python idaklu solver is removed we no longer use the python evaluator in sensitivitiy calculations, this is why some of these covereage errors. I would suggest to raise an error after line 1471 in base_solver.py (if model.calculate_sensitivities is true) saying that sensitivity calculation for the python evaluator is no longer supported, since we don't want to add tests for this. We can then remove this if statement from coverage with # pragma: no cover

Copy link
Contributor

@martinjrobins martinjrobins left a comment

Choose a reason for hiding this comment

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

thanks @pipliggins, this looks good to me. Once the conflicts are resolved it can be merged.

@pipliggins
Copy link
Contributor Author

@martinjrobins @kratman Conflicts have been resolved, this is ready to merge. Thanks!

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Thanks looks good

@kratman kratman merged commit 8084c90 into pybamm-team:develop Aug 27, 2024
25 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

remove python-idaklu
4 participants