Fixed issue #3724

[jonchapman1]

Documentation added for pybamm.Experiment.read_termination

Description

Testing the workflow for participants of the developed training workshop in April 2024 to get used to contributing with simple fixes in advance of the workshop. I added documentation for pybamm.Experiment.read_termination

Fixes #3724

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

[kratman]

If we are going to say that the type is a list[str] then we should enforce that as well. The signature can be changed to def read_termination(self, termination: list[str]): and the check for the termination being a single string can be removed.

[brosaplanella]

I think at the moment we accept both a string and a list, which is probably not ideal but there quite a few other methods work like this and changing them at the moment is a big change.

For context, this PR from Jon was to test the pre-workshop exercise instructions we are giving attendees of our next Hackathon. To ensure they know how to use Github we ask them to add a docstring somewhere and open a PR (this way we improve documentation too).

I think probably the best solution for the moment would be to write

termination : str or list[str], optional

which reflects current use.

[kratman]

If this was only to update documentation that is fine, but I would say we should open a ticket for making it just a list[str]. Long term it reduces code complexity to support only a single type of input for interfaces.

[kratman]

It is always good to clean up as we go, even for easy PRs

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (705baff) 99.59% compared to head (81578c0) 99.59%.
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3726   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          257      257           
  Lines        20802    20806    +4     
========================================
+ Hits         20718    20722    +4     
  Misses          84       84           

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

[brosaplanella]

Looks good to me, thanks Jon!

[kratman]

Removing my requested changes after discussion with @brosaplanella

I do think we should make a ticket to only support list[str] for the type though. It makes interfaces much nicer when there is only 1 input type and requires less error handling. For users this just requires putting [] around their single strings, so it is not a huge change.

[rtimms]

@all-contributors please add @jonchapman1 for docs

[allcontributors]

@rtimms

I've put up a pull request to add @jonchapman1! 🎉