pybamm-team / PyBaMM

Fast and flexible physics-based battery models in Python
https://www.pybamm.org/
BSD 3-Clause "New" or "Revised" License
1.08k stars 533 forks source link

Add base parameter class #4378

Open kratman opened 1 month ago

kratman commented 1 month ago

Description

Adds a base class for parameters and groups them into other groups aligned with BPX

Related: #3909

Type of change

Key checklist:

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

kratman commented 1 month ago

@BradyPlanden Can you take a look at this?

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.42%. Comparing base (b073fc7) to head (3848c7c). Report is 2 commits behind head on develop.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #4378 +/- ## ========================================= Coverage 99.41% 99.42% ========================================= Files 292 293 +1 Lines 22212 22493 +281 ========================================= + Hits 22083 22364 +281 Misses 129 129 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kratman commented 3 weeks ago

@brosaplanella What did you think of my groupings of parameters? Are the close enough or do they still need work?

kratman commented 3 weeks ago

To be totally honest I don't like this change. I like the split to different components, but by making it a class it feels like it moves us further away from serialization, with everything having to be defined in a class, rather than closer to it. Ideally an entire parameter set should be able to be defined in a single json file.

The way I was looking at it was that this class could be used as the interface between BPX files and the pybamm functionality. It lets us group and test if features are available. So we could easily add BPX reading/writing to the class plus sanity checking

brosaplanella commented 3 weeks ago

@brosaplanella What did you think of my groupings of parameters? Are the close enough or do they still need work?

I think they are in the right direction but work is needed. However, I believe this goes beyond parameters themselves and also covers models. For example, we have the mechanical properties, which currently are under the electrodes, but they could also fall under a new mechanics category. LAM is also another one that it is not clear. However, unless we need to set them in stone now, I am happy going ahead as things are and improving in the future.

I also agree with Valentin that serialisation and being able to define parameters as a single script should be the priority. I don't have a strong opinion regarding classes, but I think we should try to do whatever is easier for users (e.g. can we achieve similar testing results by just having some functions that read the .py or .json file and process it?).

valentinsulzer commented 3 weeks ago

I think the class is good, but parameter sets should be stored in as-close-to-json-as-possible format, not embedded into child classes and split across different methods. Right now as-close-to-json-as-possible format is python functions + dicts like we have now, since there is no good way to include the python functions in a json file.