paramm-team / pybamm-param

Parameter optimisation for PyBaMM.

BSD 3-Clause "New" or "Revised" License

38 stars 8 forks source link

Creating SPM_GITT #55

Closed muhammedsogut closed 12 months ago

muhammedsogut commented 1 year ago

Description

This PR is created to provide examples for GITT class. Currently, there's one notebook for Weepier&Huggins model but the user can benefit from a BasicGITT example.

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: $ python -m nox -s lint
[ ] All tests pass: $ python -m nox -s unit examples integration
[ ] Coverage is complete $ python -m nox -s coverage
[ ] The documentation builds: $ python -m nox -s sphinx

Further checks

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

brosaplanella commented 1 year ago

I have now updated the example. It wasn't working so in the process of debugging I realised it would be better for GITT to inherit DataFit, which also makes a lot of code redundant (I commented it out so you see what I did, but feel free to delete it once you have checked it).

Next step would be to extend the notebook to use the SPM_GITT (for that one I expect a close to perfect fit as the two models should be equivalent).

brosaplanella commented 1 year ago

@PipGrylls tests are not running on this branch, but coverage was updated. Can you take a look when you have some time?

brosaplanella commented 1 year ago

Is this work in progress or can it be reviewed?

muhammedsogut commented 1 year ago

The notebook for GITT diffusivity calculation is ready. It can be reviewed. Please let me know if there something to improve.

PipGrylls commented 12 months ago

Before we merge this can I confirm this is an extension of the GITT model with an enhanced example in GITT_loop.ipynb and doesn't require an extension to the testing?

muhammedsogut commented 12 months ago

Yes it is an updated version of GITT model and an example for it. I think GITT class might need testing.

PipGrylls commented 12 months ago

Okay I am merging loop-gitt into this branch the test written for gitt still passes but if it could merit expanding can you do that and push it to this branch then I will review it all.

PipGrylls commented 12 months ago

Infact loop-gitt seems to be redundant @muhammedsogut you are the committer on both gitt-model and loop-gitt. Can I confirm loop-gitt is not needed given this branch exists?

muhammedsogut commented 12 months ago

Yes that was created to do things done in this PR but I just used this PR and branch for them. Therefore, you can delete loop-gitt. Thanks.

muhammedsogut commented 12 months ago

Yes actually we might not need all 24 pulses. We can reduce that number in the next PR if it becomes too heavy.