Allow input parameters in ESOH model

[aabills]

Description

Allows input parameters in the ESOH model. This allows for use of the initial_soc keyword in sim.solve for variables such as Positive electrode active material volume fraction, which previously errored.

Fixes #2981

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.60%. Comparing base (c47004d) to head (9f29da8).
Report is 2 commits behind head on develop.

❗ Current head 9f29da8 differs from pull request most recent head 8b2d812. Consider uploading reports for the commit 8b2d812 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3921      +/-   ##
===========================================
- Coverage    99.60%   99.60%   -0.01%     
===========================================
  Files          259      259              
  Lines        21332    21290      -42     
===========================================
- Hits         21248    21206      -42     
  Misses          84       84              

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

[valentinsulzer]

Thanks, looks good to me

[valentinsulzer]

Can you fix coverage?

[aabills]

Can you fix coverage?

will do it tomorrow. I'm guessing that line cannot be accessed or should not be accessed.

[kratman]

Can you fix coverage?

will do it tomorrow. I'm guessing that line cannot be accessed or should not be accessed.

Breaking things into new functions tends to make it easy to cover those safety lines. Generally if you write it in a "functional" style (i.e. constant arguments into the function, returns the values, no state changes to member variables) then it gets trivial to test.

[aabills]

Can you fix coverage?

will do it tomorrow. I'm guessing that line cannot be accessed or should not be accessed.

Breaking things into new functions tends to make it easy to cover those safety lines. Generally if you write it in a "functional" style (i.e. constant arguments into the function, returns the values, no state changes to member variables) then it gets trivial to test.

Taking a look through the code, this case (known_value not in ["cell capacity", "cyclable lithium inventory"]) isn't caught anywhere else. It's pretty far outside of the scope of this PR, so I'm going to delete the extra line and move on.

This function is only 13 lines of code, do you really think it should be broken out into multiple separate functions? Or am I misunderstanding something

[kratman]

This function is only 13 lines of code, do you really think it should be broken out into multiple separate functions? Or am I misunderstanding something

I did not look at the whole function, just recommending methods for testing error handling. I will say that in general it is best to end an if-elif chain with an else. It lets future developers know that no cases were forgotten and that the intention was to end the chain that way. So my preference would be leaving the else but adding a test for it

[aabills]

This function is only 13 lines of code, do you really think it should be broken out into multiple separate functions? Or am I misunderstanding something

I did not look at the whole function, just recommending methods for testing error handling. I will say that in general it is best to end an if-elif chain with an else. It lets future developers know that no cases were forgotten and that the intention was to end the chain that way. So my preference would be leaving the else but adding a test for it

I agree, hence why I added the else in the first place, I just think it's a separate issue from this PR given that the chain in question occurs quite a few times throughout pybamm without ever having an else.

[kratman]

@abillscmu Cleanup as we go is usually a good thing, but if you don't want to do it here then can you make a ticket for it with a couple examples of places it appears?

You can assign it to me and I will refactor it later

[rtimms]

There are some other changes that would be good to make to the eSOH stuff (see #3930), so happy to make that ticket into a place to track all the refactoring needed.

[valentinsulzer]

I am happy to merge this PR once @kratman approves it

Changes were made

[kratman]

@tinosulzer All I was waiting for was a ticket for the places where @abillscmu found the missing else statements. Since he already knew where they were

[aabills]

They really aren't hard to find, all of them I know of are in pybamm/models/full_battery_models/lithium_ion/electrode_soh.py

I guess my comment

the chain in question occurs quite a few times throughout pybamm without ever having an else.

caused some confusion -- "throughout pybamm" lol is a bit of a stretch.

In any case, I added specific exceptions to a few of the higher-level occurrences of this chain that should catch all of them. Any more will likely require a more in-depth refactor, and anyone completing the refactor should have no issues finding the chains (assuming they are still there after the refactor).

[kratman]

Yeah I just wanted the ticket as more of a reminder to myself to go back and check for them

[valentinsulzer]

Looks like test failures are related to #3530 being merged in so you'll need to merge develop and fix conflicts