Added InputParameter support in PyBamm experiments

[Rishab87]

Description

Added InputParameter support in experiments, now you can pass them something like this:

current_step = pybamm.step.current(1, input_parameters=pybamm.InputParameter("I_app"), termination="2.5 V")
experiment = pybamm.Experiment(
    input_parameters={"I_app": 1},
    termination="2.5 V",
    operating_conditions=[current_step],
)

Fixes #4799

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: $ 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 -m pytest (or $ nox -s tests)
• The documentation builds: $ python -m pytest --doctest-plus src (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ 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

[valentinsulzer]

Thanks for taking this on but this isn't quite what I had in mind, the interface should be:

step = pybamm.step.current(pybamm.InputParameter("I_app"), termination="2.5 V")
experiment = pybamm.Experiment([step])
sim = pybamm.Simulation(..., experiment=experiment)
sim.solve(inputs={"I_app": 1})

[Rishab87]

made changes, now user can pass input params like this:

step = pybamm.step.current(pybamm.InputParameter("I_app"), termination="2.5 V")
experiment = pybamm.Experiment([step])
sim = pybamm.Simulation(..., experiment=experiment)
sim.solve(inputs={"I_app": 1})

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.70%. Comparing base (0d5af5c) to head (f21f01a).
Report is 3 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4826   +/-   ##
========================================
  Coverage    98.70%   98.70%           
========================================
  Files          304      304           
  Lines        23432    23452   +20     
========================================
+ Hits         23129    23149   +20     
  Misses         303      303           

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

[Rishab87]

Just a follow up anything else I need to change?

[MarcBerliner]

@Rishab87 thanks for putting this PR together! I left a few comments

[aabills]

The operator is only a property of the termination function. For example, if the termination function is "2.5 V", whether that means that the step terminates when the battery goes above 2.5 V or below 2.5 V is not well defined. The workaround for this in the past was that we could detect whether the battery was charging or discharging based on the current, and to assume that the termination would be hit if the battery goes above the voltage during charge or below during discharge. This is actually the only reason we ever needed to know the direction anyway. In the case that it is an input parameter, we can no longer calculate whether the battery is charging or discharging a priori, so instead, we have to provide an operator to fully define the event.

So for this PR, we need to ensure that any termination has an operator, because we cannot make the assumption. We probably want to keep the string parsing available, so a nice syntax would be "{operator} {value} {unit}", e.g. "< 2.5 V". But the operator must be in the termination, not the step.

[Rishab87]

@aabills I've made changes accordingly just as you told now operator gets passed in termination like "< 2.5 V". And thanks for explaining about why we need directions in first place and why we need an operator now it helped me a lot while making changes

[aabills]

Thanks! This is much closer. Just a few more small changes, and make sure you hit coverage.

[Rishab87]

@aabills thanks for the review I've added the callable case and also added few test cases accordingly to ensure full coverage

[aabills]

So close! Just one more change and this is good from my side

[Rishab87]

@aabills I've changed the code and added a check to see if symbol has input params

[aabills]

Looks good! Fix coverage and you're good to go from my side. @MarcBerliner @valentinsulzer anything else from y'all?

[aabills]

Oh and don't forget to add something to the CHANGELOG.md

[aabills]

Also fix this failing integration test.

[valentinsulzer]

looks good to me

[Rishab87]

@aabills added my PR to changelog and tests for full coverage. Also fixed the integration test

[aabills]

Looks great to me!

[MarcBerliner]

Thanks @Rishab87! Looks great