-
Notifications
You must be signed in to change notification settings - Fork 5
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
Pyfvtool implementation of spherical coordinate #43
Conversation
all advection terms (second order, upwind, and tvd) are implemented for 1D spherical grid but not yet tested
* spherical terms for advection almost complete apart from the TVD RHS for spherical 3D * wrote everything in a very sleepy condition. Needs to be checked later * need to find a proper analytical solution for tests
BCs not implemented yet
* added BC terms * added boundary values * added diffusion terms for both 1D and 3D diffusion * fixed a bug in boundary values for 1D * checked and validated the 1D formulation * TODO: divergence terms
* added divergence for 1D and 3D spherical coordinates * fixed a long-standing bug in the 3D cylindrical divergence function
* mesh attributes, cells, faces, and visualization * added few TODO ideas * still needs to be tested
* all user function calls are now in place * fixed few typos * switched off the notebook tests a they do not run on my machine * fixed a new error in bound checking of theta and phi for spherical 3D grid * enhanced the basic test to include the new spherical coordinates * fixed exceptions from not implemented to attribute error in face variable test
* fixed assignment issue in the 3D periodic BCs * fixed typo * finished the spherical diffusion example, consistent with 1D
testing advection as a simple atmospheric model * numerically unstable * an extensive validation is required
merge existing "maintenance" commits into the current "pyfvtool-spherical" development branch
* the atmospheric example works fine * a warning message for limits of theta value and boundary condition is added
* updated the notebook (will be removed later) * fixed the cell volume calculations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks fine to me. Great work!
I trust that all the formulae for the spherical terms and cell volumes are correct: I did not check those in detail, as all present tests pass, and they will surely receive future 'stress testing' when used in calculations.
I made some small changes, in particular adding some text to the 'spherical_coordinate_examples' Notebook. It provides testing of the spherical and cylindrical grids (and can be converted to a normal script for that, #44 ). I think that this Notebook can stay as an example, evolving into future documentation.
(P.S. Watch out with the slicing of the CellVariable.value
array: it is easy to mix up the dimensions, e.g. extracting a theta
slice instead of a phi
slice`. Perhaps, we should document this better, or even provide some utility functions to make it more intuitive)
Thanks, @mhvwerts for going through the code and cleaning up the code. I did not do all the derivation and did most of the work by simply looking at the conservative form of equations in spherical coordinates and comparing with the existing radial and cylindrical codes. I compared our volume calculations with the total volume of respective domains. There is a small difference between the volume of an sphere and our function, which happens only for coarse grids. Otherwise, it seems to be working. |
This PR adds the 1D and 3D spherical coordinate to the list of coordinates that can be handled by
pyfvtool
. Also closes #2.The example notebook can be removed later, and the visualization can still be improved.