Issue 1571 1 d cc opposite tabs

[rtimms]

Description

Allows tabs to be placed at the bottom of the cell in 1+1D thermal models. Adds a new Equality binary operator that returns 1 if the nodes evaluate to the same thing and 0 otherwise.

Fixes #1571

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

[codecov]

Codecov Report

Merging #1581 (eaf54f3) into develop (78b7eb6) will decrease coverage by 0.00%.
The diff coverage is 96.55%.

@@             Coverage Diff             @@
##           develop    #1581      +/-   ##
===========================================
- Coverage    97.83%   97.83%   -0.01%     
===========================================
  Files          323      323              
  Lines        17924    17927       +3     
===========================================
+ Hits         17536    17538       +2     
- Misses         388      389       +1     

Impacted Files	Coverage Δ
...m/models/full_battery_models/base_battery_model.py	99.69% <ø> (-0.01%)	⬇️
pybamm/expression_tree/binary_operators.py	99.51% <90.90%> (-0.16%)	⬇️
...icle/size_distribution/fast_single_distribution.py	100.00% <100.00%> (ø)
...e/size_distribution/fickian_single_distribution.py	100.00% <100.00%> (ø)
...mal/pouch_cell/pouch_cell_1D_current_collectors.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78b7eb6...eaf54f3. Read the comment docs.

[valentinsulzer]

Nice. You could also add the __eq__ magic function to Symbol to return Equality(a,b) for a==b

[rtimms]

I did try adding __eq__ but ran into an issue. In some places (e.g. here) we use == to check if two symbols are equal. I guess we could check the symbol id here and use == for Equality?

[valentinsulzer]

Ah right, in that case we do want to check actual equality because otherwise the clear_domain a couple of lines below causes issues. So let's leave it

[rtimms]

Hey, @tobykirk some of the many-particle submodel tests are failing on this PR, but only on certain builds (e.g. here). They pass ok locally and I haven't changed anything related to them - have you seen this error before?

pybamm.expression_tree.exceptions.ShapeError: Cannot find shape (original error: zero-size array to reduction operation minimum which has no identity)

[tobykirk]

Hey, @tobykirk some of the many-particle submodel tests are failing on this PR, but only on certain builds (e.g. here). They pass ok locally and I haven't changed anything related to them - have you seen this error before?

pybamm.expression_tree.exceptions.ShapeError: Cannot find shape (original error: zero-size array to reduction operation minimum which has no identity)

I've seen this error once before and it was similar to here: only occurring on certain builds, but I think they were different builds to these - weird!

It resolved itself after I pushed some unrelated commits, so never got to the bottom of it. Looks like something to do with set_events , i.e., the max / min concentration limits, of the many-particle submodels (FickianSingleSizeDistribution, FastSingleSizeDistribution). Actually, are these sorts of limits enforced differently now, i.e., using variable bounds? (I can't find any particle concentration events in the code for the other submodels anymore.) If so, you could try removing those events.

[rtimms]

Thanks, @tobykirk, removing set_events fixed this! As you say, they shouldn't be needed since the variable has bounds. It's weird that the test only failed sometimes, though?

[tobykirk]

Thanks, @tobykirk, removing set_events fixed this! As you say, they shouldn't be needed since the variable has bounds. It's weird that the test only failed sometimes, though?

Great! Strange error indeed, hopefully it's gone for good now.