Implement GITT example

[brosaplanella]

Description

Implement the GITT FittingProblem. I had to add a custom model for GITT and include a couple of hacks for the models to work for half-cells and I refactored the problems into a folder as there were now 4 of them (happy to revert that if you prefer).

The example is not working because it gets stuck in optimising, but produces an outcome, so I thought this would be a good time to open a PR and have some discussions.

Note this PR reuses a lot of code written by @muhammedsogut for pybamm-param

Issue reference

Fixes #223

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.
• 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 docs

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

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (de122a8) to head (d91c1e5).
Report is 19 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #249      +/-   ##
===========================================
+ Coverage    95.70%   95.80%   +0.10%     
===========================================
  Files           38       39       +1     
  Lines         2048     2097      +49     
===========================================
+ Hits          1960     2009      +49     
  Misses          88       88              

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

[NicolaCourtier]

Great work @brosaplanella and @muhammedsogut! I'm happy to review when you're ready.

[brosaplanella]

Will fix the coverage and try to get the optimiser to converge, but might need help with that. Will get in touch!

[brosaplanella]

I am not familiar with pytest so created a separate file, but it might be good to structure the tests in subfolders in the same fashion as the pybop directory.

[BradyPlanden]

Thanks for this addition @brosaplanella - overall it looks great!

I've left a few comments to look at, the biggest being the addition of the GITT class. I think in this situation it can be summarised into an example with the fitting class and either diffusion parameter. Happy to chat further about this.

[NicolaCourtier]

Nice work @brosaplanella! I've added a couple of comments to Brady's review.

In short, I think this is a great structure to build upon. I can imagine the GITT problem class being extended in a few useful ways in future, so I would keep it separate from the example script. For example, we could add processing of multiple pulses and more user guidance/warnings relating to different models.

The one change I would like to see is for the model to be defined in the example script and passed as an input to the GITT problem, for consistency with our model-problem-cost-optimisation workflow. To restrict the GITT fitting process to the WeppnerHuggins model only, the initialisation step could throw a warning for any other model input with a short explanation to the user.

[BradyPlanden]

Here's the example implementation using the FittingProblem (c55b2a3). Let's chat at OBMS about next steps, this discussion has been great and I think we are in need of an updated design philoshophy document moving forward. Thanks!

[BradyPlanden]

I'm conscious that this PR has had a blocker on the addition of a GITT class. To move this forward, can this be implemented as an example (as shown above) and I will open an issue on how we move forward with the GITT class. I think it has potential, but I would like to see a bit more on what additional features it would have over FittingProblem, but I don't want to hold up this PR any longer. Let me know if you are tied up @brosaplanella and I can help make these changes.

[NicolaCourtier]

@BradyPlanden I think it is reasonable to add GITT as a Problem subclass in this PR - providing user guidance on the GITT fitting procedure is within scope of the PyBOP aims and creating the class provides a place to store and develop this guidance.

[BradyPlanden]

Following up on our discussion from yesterday, we agreed to move the GITT class into an example script (or notebook). In addition adding references pb-param where applicable in the docstrings of these classes/methods.

[brosaplanella]

This is ready now. Unfortunately the codecov failed, so I can't see the coverage (I suspect tests are missing but not sure where). Will try to debug locally.

EDIT: tried running the coverage locally and I get a test failing, so don't get a report unfortunately. Codecov says my branch is covered (https://app.codecov.io/gh/pybop-team/PyBOP/tree/brosaplanella%2FPyBOP%3Aissue-223-GITT/pybop%2Fmodels%2Flithium_ion), so if we can't get the report to work on this PR we can always merge and I am happy fixing the coverage as a separate PR if there are any issues.

[NicolaCourtier]

Nice additions, thanks @brosaplanella !

[BradyPlanden]

I retriggered the coverage test and it seems to have worked this time. Let's see if it continues to fail sporadically, if so I'll open an issue and we'll sort it out.

Thanks for updating so quickly, if you can get the coverage up that would be great. I think you should be able to add the additional tests to the unit/test_models.py file.

[brosaplanella]

About the default parameters for the Weppner&Huggins model, the main difference is that it uses a linearised OCP. I think the choice boils down to:

1. Do we want users to provide the reference OCP and slope (i.e. the two coefficients in the linear approximation)?
2. Do we want to compute them automatically from the OCP curve and the initial concentration (i.e. perform the linearisation internally)?

1 gives users more control, but would require me tinkering with the default parameters, 2 would work with any of the existing PyBaMM parameter sets.

For now, it might be better to go with 1, and we can always support 2 in the future. Thoughts?

[NicolaCourtier]

Happy to go with your decision on this @brosaplanella - my thoughts would be that (2) would be the desirable default in future, with the option to override the computed values via (1).

[brosaplanella]

my thoughts would be that (2) would be the desirable default in future, with the option to override the computed values via (1).

I totally agree! I would like to implement some of the more advanced models for GITT, which I think they will give a better perspective on how to proceed, and we can address them as a future PR.

[BradyPlanden]

Excellent - nice one @brosaplanella. LGTM!

[BradyPlanden]

@all-contributors please add @muhammedsogut for code.

[allcontributors]

@BradyPlanden

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