Add base parameter class

[kratman]

Description

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

Related: #3909

Type of change

• New feature (non-breaking change which adds functionality)

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]

@BradyPlanden Can you take a look at this?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

@@            Coverage Diff            @@
##           develop    #4378    +/-   ##
=========================================
  Coverage    99.41%   99.42%            
=========================================
  Files          292      293     +1     
  Lines        22212    22493   +281     
=========================================
+ Hits         22083    22364   +281     
  Misses         129      129            

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

[brosaplanella]

Just a picky notation comment, but looks good, thanks! Make sure to include a line in the CHANGELOG before merging.

[brosaplanella]

Why is it AbstractBaseParameters and not just BaseParameters? If anything, these would be battery parameters (rather than abstract parameters). Also, might be worth to use BaseParameterSet or BaseParameterValues instead. I think we are not 100% consistent with this, but in theory parameters are the concept (e.g. diffusion coefficient) while parameter values/sets are the value.

[kratman]

Abstract as in an abstract class where implementation details are not fully defined. The idea was that this provides an interface that we can extend for the various parameter sets.

[valentinsulzer]

+1 for changing to AbstractBaseParameterSets

[kratman]

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

[valentinsulzer]

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.

[kratman]

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]

@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]

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.