Issue 997 curr coll

[rtimms]

Description

Updates effective current collector models and adds a notebook comparing pouch cell models. I've put in a little workaround for processing variables that depend on space only, but will fill this properly as a separate issue (#1006).

Fixes #997

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: $ flake8
• All tests pass: $ python run-tests.py --unit
• The documentation builds: $ cd docs and then $ make clean; make html

You can run all three 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]

Looks great, thanks! I haven't checked the model equations in fine detail but given that the results look good I assume it's fine.

I got a bit confused by what EffectiveResistance did (for a while I thought it was a 0D model). I think it would be easier to understand if that model took an option dimensionality which specifies 1D behaviour vs 2D behaviour, instead of subclassing. This is more consistent with how we have been reformatting the submodels

[valentinsulzer]

why does the base model now take options?

[rtimms]

I wanted to make it so you could use base model in simulations (simulation does new copy with options), but I see now what I have done isn't quite right

[valentinsulzer]

I'm surprised this works as I thought self.options=options called the options setter method?

[valentinsulzer]

typo: as points in space

[valentinsulzer]

these refs can go at the top under the paragraph where they are introduced

[valentinsulzer]

interp.interp1d( space, entries_for_interp[:, 0], kind="linear", fill_value=np.nan) should be outside the function so that it's not recomputed each time

[rtimms]

thanks!

[valentinsulzer]

maybe emphasize potential and current density in this docstring

[rtimms]

good point

I got a bit confused by what EffectiveResistance did (for a while I thought it was a 0D model). I think it would be easier to understand if that model took an option dimensionality which specifies 1D behaviour vs 2D behaviour, instead of subclassing. This is more consistent with how we have been reformatting the submodels

Agreed, will change to this. Also, in this case these models will then work with simulation, so maybe best to leave base model alone

[valentinsulzer]

Looks great, thanks!

[valentinsulzer]

missing "Parameters" section title here