Issue 771 secondary broadcast#776
Conversation
Codecov Report
@@ Coverage Diff @@
## master #776 +/- ##
==========================================
+ Coverage 98.38% 98.42% +0.04%
==========================================
Files 179 179
Lines 9916 9931 +15
==========================================
+ Hits 9756 9775 +19
+ Misses 160 156 -4
Continue to review full report at Codecov.
|
rtimms
left a comment
rtimms
left a comment
There was a problem hiding this comment.
looks great, thanks @tinosulzer ! just a few comments about docstrings/comments. not for this issue, but it might helpful to have a notebook that shows what the different broadcast types do, as i'm not sure it's obvious to new users why you need the different cases
|
|
||
| class PrimaryBroadcast(Broadcast): | ||
| """A node in the expression tree representing a primary broadcasting operator. | ||
| Broadcasts in a `primary` dimension only. That is, makes explicit copies |
There was a problem hiding this comment.
I'm not sure the "That is, makes explicit copies" is particularly clear?
There was a problem hiding this comment.
oh yeah I got distracted while writing this 😅
| # electrode to particle | ||
| if child.domain == []: | ||
| pass | ||
| elif child.domain[0] in [ |
There was a problem hiding this comment.
can we do a similar check that if the domain is current collector, the broadcast domain must be and electrode or separator?
| class PrimaryBroadcast(Broadcast): | ||
| class SecondaryBroadcast(Broadcast): | ||
| """A node in the expression tree representing a primary broadcasting operator. | ||
| Broadcasts in a `primary` dimension only. That is, makes explicit copies |
There was a problem hiding this comment.
"secondary dimension only", and as above the last part of this isn't clear to me
| "See :meth:`Broadcast.check_and_set_domains`" | ||
|
|
||
| # Can only do secondary broadcast from particle to electrode or from | ||
| # current collector to electrode |
There was a problem hiding this comment.
from electrode to current collector?
| "Clear domains, bypassing checks" | ||
| self._domain = [] | ||
| self._auxiliary_domains = {} | ||
|
|
| self.first_dimension = "r" | ||
| self.second_dimension = "x" | ||
| self.r_sol = first_dim_pts | ||
| self.x_sol = second_dim_pts |
valentinsulzer
left a comment
valentinsulzer
left a comment
There was a problem hiding this comment.
Have updated the docstrings and made the notebook, let me know if it's clearer
|
yep, this looks great thanks. the notebook is very clear! |
2 participants
Description
Reformat broadcasting to make sure it's always done in the right order and get rid of hacks
Fixes #771
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.
Key checklist:
$ flake8$ python run-tests.py --unit$ cd docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: