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

Take r_average of a variable that is broadcast #1114

Closed
weilongai opened this issue Jul 24, 2020 · 8 comments · Fixed by #1118
Closed

Take r_average of a variable that is broadcast #1114

weilongai opened this issue Jul 24, 2020 · 8 comments · Fixed by #1118
Assignees

Comments

@weilongai
Copy link
Contributor

Describe the bug
This is a question rather than a bug. How to show the value of variables (e.g. the r-averaged concentration c_s_av as below) in debug mode (in VS code)? This is convenient to check the definition of equations and to dig out the issues as a developer.

Another question is how to get the r-averaged concentration for all electrode particles in a P2D model? It only has x-average concentration of c_s_n_xav and c_s_p_xav in standard_variables.py

To Reproduce
Steps to reproduce the behaviour:

  1. Set a breakpoint to Line 37 in base_particle.py
  2. Debug SPMe.py, then it will stop at the breakpoint in the last step.
  3. Watch the variable c_s_av, then it shows a list of features but not the values.

Expected behaviour
To show a matrix/array of values for the average concentration for electrode particles.

Screenshots
image

@valentinsulzer
Copy link
Member

When the model is being built, everything is purely symbolic so the value of the concentration doesn't exist. Typically what I do is solve the model and then look at the values of the variables. What is the reason for needing to see the value before solving?
It looks like we never compute the r-average, I guess we never needed it until now. It should be added to _get_standard_concentration_variables in base_particle.py, like this:

"R-averaged "
+ self.domain.lower()
+ " particle concentration": pybamm.r_average(c_s)

@weilongai
Copy link
Contributor Author

Thanks for your reply. For some intermediate variables defined in functions but not for outputs, can I still see the values in the solution? I was trying to figure out where was the issue when getting a wrong result, by checking the values of intermediate variables.

Following your explanation on the r-average, is it applicable to many particles?

@valentinsulzer
Copy link
Member

You would need to define the intermediate variables and add them to the list of variables. If it doesn't make sense to add them to the dictionary of variables, you could do it in your main script e.g.

c = model.variables["Electrolyte concentration"]
model.variables["Two times c"] = 2*c

then they will appear in the final solution.

Many-particle models aren't implemented yet, but r_average should work when c_s depends on both x and r

@weilongai
Copy link
Contributor Author

I have tried to get the r_average concentration, but it fails to broadcast the variable to the domain negative electrode. Besides, is it applicable to the DFN, as I'm currently using the SPM? Thanks.

I used the commands:
c_s_n = variables["Negative particle concentration"]
c_s_n_avg_p = pybamm.r_average(c_s_n) # average concentration for particles
c_s_n_avg = pybamm.SecondaryBroadcast(c_s_n_avg_p,["negative electrode"])

And when I printed their domain, I got
c_s_n.domain
['negative particle']
c_s_n_avg_p.domain
['negative particle']
c_s_n_avg.domain
['negative particle']

@valentinsulzer
Copy link
Member

In theory, you shouldn't need to broadcast it again as c_s_n already depends on x and r, so just taking the r_average would make it depend only on x. But I have just noticed there is a bug in r_average in the lines

# If symbol is a Broadcast, its average value is its child
elif isinstance(symbol, pybamm.Broadcast):
    return symbol.orphans[0]

It does this even if the symbol is broadcast in the x-direction, which will be the case if you are using SPM. So r_average should be edited to deal with that case. More specifically:

  • c_s_n is pybamm.PrimaryBroadcast(c_s_n_xav, "negative electrode") where c_s_n_xav depends only on r.
  • in this case, r_average should return pybamm.PrimaryBroadcast(r_average(c_s_n_xav), "negative electrode")

@valentinsulzer valentinsulzer changed the title How to show the value of variables in debug mode? Take r_average of a variable that is broadcast Jul 27, 2020
@weilongai
Copy link
Contributor Author

weilongai commented Jul 27, 2020

Thanks for your reply. I need to report another bug (I think) below, which you may correct at the same time.

The child of c_s_n = variables["Negative particle concentration"] is Variable(0x2c798945b2921b7, Negative particle surface concentration, children=[], domain=['negative electrode'], auxiliary_domains={'secondary': "['current collector']"}).

So pybamm.r_average(c_s_n) turns out to be Negative particle surface concentration following the if condition above, which is not right. I'm testing with DFN this time.

@valentinsulzer valentinsulzer self-assigned this Jul 29, 2020
@valentinsulzer
Copy link
Member

I can't reproduce this. When running the normal DFN, the variable c_s_n like you defined gives

Variable(-0x6f6b862f45774429, Negative particle concentration, children=[], domain=['negative particle'], auxiliary_domains={'secondary': "['negative electrode']", 'tertiary': "['current collector']"})

with no children.

The way I can get it to do what you reported is if I run the DFN with options {"particle": "fast diffusion"}, in which case that is expected behaviour (solve an ODE for average particle concentration which is the same as surface particle concentration). Were you doing this, or something different?

valentinsulzer added a commit that referenced this issue Jul 29, 2020
valentinsulzer added a commit that referenced this issue Jul 29, 2020
@weilongai
Copy link
Contributor Author

Yes, I was using the fast particle submodel, which is the default option from a notebook example. The Fickian type model should be used instead. Thanks for pointing this out.

valentinsulzer added a commit that referenced this issue Jul 29, 2020
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