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?

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

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.

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

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.

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.

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

yep looks like a typo to me

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

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

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?

Yes, that should be correct

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

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)

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?

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

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