Issue 2322 experiment steps

[valentinsulzer]

Description

Reformat the backend for specifying experiment steps.

The underlying (private) class for steps is called _Step. This class should never be called directly, instead the steps of an experiment can be specified via helper functions as follows:

1. "Discharge at 1C until 2.5 V": As before, strings of this kind can be used, and behavior will be unchanged. However, some formats are no longer supported (see below). For those formats, the other helper functions should be used.
2. pybamm.experiment.string should be used to specify a string with more options:

• the period argument replaces putting "(10 second period)" in the string (e.g. `period="10 minutes")
• the temperature argument replaces putting "at 25 oC" in the string (e.g. `temperature="25 oC")
• the tags argument replaces tagging a string with "[tag1]" (e.g. tags=["tag1"])
  For example, the string "Discharge at 1C until 2.5V at 30oC (1 second period) [tag1]" is now pybamm.experiment.string("Discharge at 1C until 2.5V", period="1 second", temperature="30oC", tags="tag1"). Period can be a string or a float (in which case it is in seconds). Temperature can be a string or a float (in which case it is in Kelvin). Tags can be a single string or a list of strings.
  If a string does not need any extra options, the string can be passed directly to pybamm.Experiment without using the pybamm.experiment.string function, i.e. the following experiments are equivalent

pybamm.Experiment(["Discharge at 1C for 1 hour"])

and

pybamm.Experiment([pybamm.experiment.string("Discharge at 1C for 1 hour")])

3. We have added current/C-rate/voltage/power/resistance methods, for programmatically specifying experiments without having to use string formatting. For example,

pybamm.experiment.current(1, duration="1 hour", termination="2.5 V")

is equivalent to "Discharge at 1A for 1 hour or until 2.5V". The period, temperature, and tags options are the same as for pybamm.experiment.string.
These methods can also be used for drive cycles:

drive_cycle_power = ... # load numpy 2-column array
experiment = pybamm.Experiment([pybamm.experiment.power(drive_cycle_power)])

The functionality pybamm.Experiment(["Run US06 (W)"], drive_cycles={"US06": drive_cycle}) has been deprecated

Fixes #2322 #2982

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 (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
• The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

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

[valentinsulzer]

It was just getting a bit out of hand with the string parsing. I think it's reasonable to have basic instructions being a string and having to use the experiment.string class so specify further options. But this is a big change so I do want to make sure we do it properly and make sure people are ok with this change.

[brosaplanella]

Looks good to me, thanks! It would be good to have another reviewer's opinion before merging, at least about the API. Having slept on it, the approach on what is allowed as a string makes sense (in my mind: basic functionality can be passed as string, advanced functionality requires using the new classes).

In any case, we can always bring existing functionality into the string if we see a big demand.

EDIT: before merging please add line to CHANGELOG

[TomTranter]

I think it would be clearer to use "instruction" or "step" instead of experiment with lower case "e" for the new steps of the Experiment. Or maybe even make adding steps a method of the Experiment class. Also some more documentation on the termination conditions that can be applied to both the experiment._Step and the Experiment and how they interact would be good. I don't think we should be allowing ambient temperature to get over-written at the step level either. That feels like a top level control condition and also a bit of a hack that should really just be part of an input variable

[RuiheLi]

Hi all, I reckon that I am still in learning and adapting these discussions and the webpage of GitHub (there are so many buttons and information here), all the changes make sense to me as being part of the team to implement this new function.
I can see now some checks were unseccessful, I also run python run-tests.py --all locally and it says:
Ran 1550 tests in 823.043s FAILED (failures=1, errors=2, skipped=63)
@brosaplanella when would be a good time for you next week for a one-to-one meeting to discuss how to resolve these unsuccessful checks? Thanks!

[TomTranter]

@brosaplanella are you looking into the benchmark regression before merging?

[brosaplanella]

@brosaplanella are you looking into the benchmark regression before merging?

It seems it's only one benchmark failing which could be a random fail:

27.0±0.1ms      43.9±0.06ms     1.62  different_model_options.TimeSolveLossActiveMaterial.time_solve_model(<class 'pybamm.models.full_battery_models.lithium_ion.spm.SPM'>, 'reaction-driven', <class 'pybamm.solvers.casadi_solver.CasadiSolver'>)

Rerunning the benchmarks now to see if that's the case.

[brosaplanella]

Now another one fails but it is a small regression, so happy to merge. I think @tinosulzer wanted to bring the period back in though...