PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Non-spherical particles

Open DrSOKane opened this issue 5 years ago • 17 comments

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.

DrSOKane avatar Jun 23 '20 16:06 DrSOKane

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?

valentinsulzer avatar Jun 23 '20 16:06 valentinsulzer

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

DrSOKane avatar Jun 23 '20 16:06 DrSOKane

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.

DrSOKane avatar Jun 23 '20 16:06 DrSOKane

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

valentinsulzer avatar Jun 23 '20 16:06 valentinsulzer

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 avatar Jun 24 '20 09:06 DrSOKane

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.

DrSOKane avatar Jul 10 '20 15:07 DrSOKane

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

valentinsulzer avatar Jul 10 '20 18:07 valentinsulzer

yep looks like a typo to me

rtimms avatar Jul 10 '20 20:07 rtimms

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

rtimms avatar Jul 10 '20 20:07 rtimms

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

valentinsulzer avatar Jul 10 '20 23:07 valentinsulzer

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?

DrSOKane avatar Jul 13 '20 09:07 DrSOKane

Yes, that should be correct

valentinsulzer avatar Jul 13 '20 12:07 valentinsulzer

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...

DrSOKane avatar Jul 13 '20 14:07 DrSOKane

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)

valentinsulzer avatar Jul 13 '20 18:07 valentinsulzer

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?

katiezzzzz avatar Jul 17 '20 12:07 katiezzzzz

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

valentinsulzer avatar Jul 17 '20 21:07 valentinsulzer

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

valentinsulzer avatar Aug 04 '20 14:08 valentinsulzer