pybamm-team / PyBaMM

Fast and flexible physics-based battery models in Python
https://www.pybamm.org/
BSD 3-Clause "New" or "Revised" License
1.12k stars 547 forks source link

Non-spherical particles #1072

Open DrSOKane opened 4 years ago

DrSOKane commented 4 years ago

Summary Include other geometries for electrode particles

Motivation Tomography and SEM experiments agree that graphite particles are not spherical.

Additional context I have devised a model for graphite particles that are coin-shaped, with (de)intercalation at the edge of the coin, as opposed to spherical, but have not had a chance to test it yet.

valentinsulzer commented 4 years ago

This should be ok to implement. Mainly requires adding cylindrical operators for grad, div and integral. How do you define surface area to volume ratio in this case?

DrSOKane commented 4 years ago

Circumference divided by area, i.e. 2eps_s/r_p, as opposed to the 3eps_s/r_p for spherical particles

DrSOKane commented 4 years ago

I was actually proposing non-spherical particles as a summer project for an undergraduate student. I mainly just wanted to check it wasn't already being done.

valentinsulzer commented 4 years ago

Sounds good. Not being done as far as I'm aware. Would "cylindrical particles" be a better name for the issue? Since "non-spherical" can mean anything.

I can't tell if you are proposing to do this in your fork of pybamm, or somewhere else first, but in pybamm it should be quite simple:

  1. define a new SpatialVariable whose coordinate system is "cylindrical polar" instead of "spherical polar"
  2. add an option to the battery_geometry function to use this "cylindrical polar" variable instead of the existing spherical polar one when defining the mesh
  3. add a case to finite volumes for how to define grad, div, integral when the coordinate system is "cylindrical polar"

then you can just replace the default geometry with the cylindrical one and it should work

DrSOKane commented 4 years ago

Thanks Tino. My proposed geometry is actually circular (2-D) as opposed to cylindrical, as I'm assuming the thickness of the coin to be negligible. I chose the title "Non-spherical particles" as others may want to implement different geometries; circular graphite just happens to be what interests me.

DrSOKane commented 4 years ago

The student and I have started coding the cylindrical geometry, but there's the line in the function integral in finite_volume.py corresponding to the normal spherical geometry that's confusing us:

out = 4 * np.pi * 2 integration_vector @ (discretised_child * r)

We don't understand why it's not

out = 4 np.pi integration_vector @ (discretised_child * r ** 2)

Until we understand how the normal spherical integral works, we can't implement the cylindrical one.

valentinsulzer commented 4 years ago

Oooops, that looks like a typo, right @Scottmar93 @rtimms ?

rtimms commented 4 years ago

yep looks like a typo to me

rtimms commented 4 years ago

presumably there is a typo in the tests too, as this should’ve been picked up?

valentinsulzer commented 4 years ago

Yes the tests are wrong too. I am an idiot, as beautifully documented in #293 . Thanks very much for picking this up @DrSOKane !

DrSOKane commented 4 years ago

So what is the correct formulation? Mathematically, the integral is

\int_0^1 4 \pi r^2 \,\mathrm{d}r

using LaTeX notation. Does this translate to

out = 4 np.pi integration_vector @ (discretised_child * r ** 2)

in PyBaMM?

valentinsulzer commented 4 years ago

Yes, that should be correct

DrSOKane commented 4 years ago

What is the integral used for anyway? I ask because we're treating the height of the cylinder as infinitesimally small, but I don't imagine your code will like that...

valentinsulzer commented 4 years ago

The spherical integral only comes into the model through the r_average function (here), which returns Integral(symbol, r) / Integral(1, r). So the height of the cylinder would cancel out anyway, and it should be fine to just do out = 2 * np.pi * integration_vector @ (discretised_child * r)

katiezzzzz commented 4 years ago

Hi Pybamm team and thanks @tinosulzer for your help! I am the undergraduate student who is working with @DrSOKane on adding the geometry. I have made a few modifications to battery_geometry, standard_spatial_vars and finite_volume files locally in order to add this geometry. Will it be ok to push these changes after testing?

valentinsulzer commented 4 years ago

Yep, sounds great, thanks! Just open a pull request when you are ready

valentinsulzer commented 4 years ago

The issue with the spherical integral has now be fixed separately and merged into develop. Thanks again for pointing it out