Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow variables to have more than 3 domains #1580

Closed
tobykirk opened this issue Jul 29, 2021 · 4 comments · Fixed by #1590
Closed

Allow variables to have more than 3 domains #1580

tobykirk opened this issue Jul 29, 2021 · 4 comments · Fixed by #1590

Comments

@tobykirk
Copy link
Contributor

Summary
Currently variables can have a maximum of 3 domains, i.e. a "primary" domain and two auxiliary domains, "secondary" and "tertiary". This could be extended to allow "quaternary" etc. domains (or changed to allow any number of domains).

Also, a TertiaryBroadcast() (or a generalization) could be defined, similar to PrimaryBroadcast() and SecondaryBroadcast().

Motivation
Necessary in order to add particle-size distributions to the DFN framework, where some variables live on 4 different domains , e.g.

 c_s_distribution = pybamm.Variable(
    "Negative particle concentration distribution",
    domain="negative particle",
    auxiliary_domains={
        "secondary": "negative particle size",
        "tertiary": "negative electrode",
        "quaternary": "current collector",
    },
    bounds=(0, 1),
)

(Even if some domains are 0D in practice, they can push others off the domain list before the domains are discretized.)

A TertiaryBroadcast() would then be used for e.g. broadcasting the "X-averaged" concentration distribution to the "electrode" domains (which would be added in the "tertiary" position).

Additional context
Add any other context or screenshots about the feature request here.

@valentinsulzer
Copy link
Member

valentinsulzer commented Aug 2, 2021

Not sure TertiaryBroadcast is needed. SecondaryBroadcast broadcasts from a lower dimension to a higher dimension (e.g. particle to electrode) and PrimaryBroadcast broadcasts the other way. The names are definitely misleading, I think originally they were intended differently

@tobykirk
Copy link
Contributor Author

tobykirk commented Aug 2, 2021

I see @tinosulzer. I could be mistaken, but it looks to me that SecondaryBroadcast always puts the higher dimension broadcast domain in the secondary position (shifting previous secondary domain to tertiary). For a max of 3 domains, this basically covers all the higher dimension broadcast cases (as variables either already have the current collector as a domain, or no domains at all in which case you'd use FullBroadcast instead).

But, a new use-case with the size domain would be, e.g. starting from a symbol with domains

domains = {
    "primary": "negative particle",
    "secondary": "negative particle size",
    "tertiary": "current collector"
}

and broadcasting to "negative electrode", ending up with

domains = {
    "primary": "negative particle",
    "secondary": "negative particle size",
    "tertiary": "negative electrode",
    "quaternary": "current collector"
}

Is this possible with SecondaryBroadcast currently? If not, could change it to put the broadcast domain in an auxiliary position of choice, or just put it where we know it should go (electrode always after particle size, particle, etc)? Maybe motivates renaming to something like AuxiliaryBroadcast, though.

@tobykirk
Copy link
Contributor Author

tobykirk commented Aug 2, 2021

I'm willing to take a shot at this (adding quaternary domains), but any other suggestions of how to implement this are definitely welcome!

@valentinsulzer
Copy link
Member

Oh yeah I think you're right. Secondary broadcast is actually for squeezing in a domain in the 2nd position and moving 2nd position to 3rd position, so we do need a new tertiary. Definitely good to have someone else look at this part of the code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants