paramm-team / pybamm-param

Parameter optimisation for PyBaMM.
BSD 3-Clause "New" or "Revised" License
38 stars 8 forks source link

Refactor `OCPBalance` to align with `DataFit` #42

Closed brosaplanella closed 1 year ago

brosaplanella commented 1 year ago

At the moment OCPBalance is very different from DataFit which makes it hard to maintain. It should be reformatted so it is more similar (e.g. in the use of the parameters_optimise, cost_function_parameters, etc.).

PipGrylls commented 1 year ago

Hi, Ahead of our meeting on Wednesday I've started looking at this as it looked like a good way to get to know the codebase and use the examples.

brosaplanella commented 1 year ago

Just realised that many things will change once #32 is merged, so might be better to do that first. If you want, you can review that PR first. I have now added you to the team so you should be able to leave a review.

PipGrylls commented 1 year ago

Ah thanks for the heads up I will have a look

PipGrylls commented 1 year ago

Changes pulled from main (post main updated with #32) https://github.com/paramm-team/pybamm-param/commit/e3e20ff5388a8e000baaa0b8669930d1f47e1264

PipGrylls commented 1 year ago

In the base class __init__ we define the following variables:

def __init__(self):

        """
        Initialize the class properties

        x0 : array-like
            Initial guess for the optimization problem
        bounds : tuple
            bounds of the optimization problem
        scalings : array-like
            scalings for the optimization problem
        """
        self.x0 = None
        self.bounds = None
        self.scalings = None

OCPBalance does use super().init(), data fit does not.

We then have some disconnect between the names of input data and reference data.

I will try and pull these things together, to fulfil the brief of this issue but additionally I think there needs to be some strict templating applied to how further optimisation problems are defined.

We can choose the direction the templating goes in but making it permissive will make long term upkeep harder due to edge cases.

brosaplanella commented 1 year ago

I agree. I am away this week, but will think about this and we discuss when I am back.

brosaplanella commented 1 year ago

I have revisited the two classes (plus the new GITT one we are starting to develop) and they all have the following common features. The first column are the abstract concepts needed to define the class, while the value in each cell is how these are passed/defined for the optimisation problem of the respective column.

DataFit OCPBalance GITT
model simulation data_fit choose from given
data data data_ref data
parameters model_parameters fixed fixed
variables fit variables_optimise fixed fixed
cost function cost_function cost_function cost_function

The various classes might have inputs that are specific to them (e.g. GITT will require some parameters).

Then, the __init__ would have to:

The specific methods will be different for each class, but we could just call process_model, etc. from the base class and define what the function does in each subclass.

PipGrylls commented 1 year ago

Thats really useful thanks.

I think there is some argument to include some sphinx docs (new issue needed for this), pybamm uses them so it would be consistent.

brosaplanella commented 1 year ago

There are some docs here (https://pybamm-param.readthedocs.io/en/latest/?badge=latest) but there is a lot that can be improved.

PipGrylls commented 1 year ago

Gitt pulled from branch 26-implement-gitt, then updated to make naming self similar. This commit, 2335211, supersedes PR #45