Issue 2111 custom transport efficiency

[TomTranter]

Description

Adding new methods for calculating the transport efficiency beyond Bruggeman coefficient. This includes using the following relations for tortuosity factor ($\tau$) for use in transport efficiency $B=\epsilon/\tau$:

• Tortuosity factor: allows direct specification of a tortuosity factor
  These were taken from 10.1016/j.ces.2007.03.041 and http://dx.doi.org/10.1016/j.coche.2016.02.006

Fixes #2111

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

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[TomTranter]

This line looks to be wrong as active material won't always be the only solid conducting material. Using 1-porosity would be better

PyBaMM/pybamm/models/submodels/transport_efficiency/bruggeman_transport_efficiency.py

Line 39 in bd5e240

eps_k = variables[f"{Domain} active material volume fraction"]

[brosaplanella]

This line looks to be wrong as active material won't always be the only solid conducting material. Using 1-porosity would be better

PyBaMM/pybamm/models/submodels/transport_efficiency/bruggeman_transport_efficiency.py

Line 39 in bd5e240

eps_k = variables[f"{Domain} active material volume fraction"]

Agree (though I personally think that we should not use transport efficiency in the electrode given that what we measure is directly the effective conductivity). But that's a separate issue probably, so happy to have 1 - porosity for now.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.58%. Comparing base (c687134) to head (156a5f5).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3437      +/-   ##
===========================================
- Coverage    99.58%   99.58%   -0.01%     
===========================================
  Files          261      268       +7     
  Lines        21454    21673     +219     
===========================================
+ Hits         21366    21582     +216     
- Misses          88       91       +3     

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

[agriyakhetarpal]

Sorry, I forgot to give an update here but I did try this locally in the last week of November after I said I will. The error message was painfully difficult to debug, and I tried an array of approaches to fix it but resorted to leaving it be later on: moving the paths for various files, excluding all new files from this PR from the docs generation, experiments with switching to an alternative PDF builder (rinohtype), and more – nothing has worked so far.

However, I have been able to isolate that the problem is not with the notebooks (they are not processed during the PDF builds), and not with the directives in the .rst files – which suggests that this is coming from either how the file structure is distributed for the submodel or from something we missed in the docstrings for the submodel classes.

@tinosulzer had suggested enabling PDF builds when I was a GSoC participant, and so were they done soon enough, but we can look into removing them if we do not wish to include them again. Please let me know what works, in the meantime I can take another look at this on Monday.

[DrSOKane]

As someone who tried implementing the user-supplied tortuosity factor myself, this is very good to see!

The problem I quickly ran into with non-Bruggeman tortuosity models is how to tell PyBaMM to disregard the tortuosity for solid-phase conductivity. With Bruggeman models it is done by specifying an exponent of 0, but this doesn't work for any of the other models. The only solutions I can think of are:

• Specify separate options for electrolyte and electrode transport efficiency, which is a lot of work for little gain
• Specify a "tortuosity for electrode conductivity" option which can only be true or false
• Parameter sets corresponding to paper that use non-Bruggeman tortuosity models should have the initial electrode conductivity adjusted accordingly

Furthermore, using phi = 1 - eps is incorrect for models that feature degradation. The tortuosity will decrease as the SEI grows, and LAM is not accounted for.

These are minor issues that will be easy to solve if everyone can agree on which approach to use.

[TomTranter]

Hi @DrSOKane thanks for your comments and good suggestions. I think the best approach is "Specify separate options for electrolyte and electrode transport efficiency". It is perfectly valid to have different functional relationships for each phase. On the point of LAM I would need to dig into this but we should certainly account for it. We should Ideally have a volume fraction model that can account for active material, binder, SEI, non-active material (broken-off chunks) and porous space

[TomTranter]

@agriyakhetarpal thanks for he update - it certainly is a hard problem to find and fix. Maybe @martinjrobins has experience that can help or I could contact the readthedocs maintainers

[agriyakhetarpal]

I agree. Though, it doesn't look to me as something related to RTD (fails locally as well), so we would rather be better off contacting the Sphinx developers – though I can't seem to find the related issue at this moment, it had been dormant for years the last time I looked ☹️

[TomTranter]

@agriyakhetarpal I found the source of the docs errors finally. It was missing footbibliography commands on the rst files

[agriyakhetarpal]

Thanks, @TomTranter! That makes so much more sense now, I imagine it was surely not easy to find out the source of it through the unhelpful error message :)

[TomTranter]

@brosaplanella can we merge this in now? @DrSOKane made a suggestion for a code change but I don't have time to implement that right now and would rather get this is now it's passing

[brosaplanella]

Looks good to me, thanks! Coverage is not passing, but I can't see any file with missed lines so we should be fine to merge anyway.

[brosaplanella]

I can't merge as it needs Simon's approval, given that he requested changes. @DrSOKane can you check if you are happy with it?

[DrSOKane]

I'm still not happy with the assumption that phi = 1 - eps. However, this is badly needed for the next release, so I will approve it. The binder can be added explicitly in a future PR.