Planetary radius#2739
Conversation
…tary radius at this time.
…n for different input args (string vs SphericalMesh object, radius given or not), short advection test with radii None, Mars, Venus, Earth.
…non-numeric, negative, or 0. Added matching tests to test_mesh. Co-authored by: Claude <noreply@anthropic.com>).
for more information, see https://pre-commit.ci
erikvansebille
left a comment
erikvansebille
left a comment
There was a problem hiding this comment.
Great work, @maureenjcohen! Just a few comments and suggestions below
We should also make sure to highlight this new feature in the documentation; perhaps in the Grids tutorial that @michaeldenes is working on? Can you do that, @michaeldenes?
| if self._radius is None: | ||
| return 1852 * 60.0 | ||
| else: | ||
| return self._radius * np.pi / 180.0 |
There was a problem hiding this comment.
This may be a bit overcomplicated. Why not simply use Earth radius as default (6366707.019493707?), so that its deg2m ends up being 1852 * 60? (same for xgrid below)
There was a problem hiding this comment.
An advantage would then also be that a radius is None could indicate a flat Mesh, so that we don't need to check for the string mesh == flat but instead radius is None? But that may be another PR?
There was a problem hiding this comment.
I wasn't sure if the exact 1852 * 60 was needed, or if using Earth radius might lead to small inconsistencies when switching between v3 and v4. I think your suggestion is good.
There was a problem hiding this comment.
But if the user doesn't specify a radius, should radius default to None (flat mesh) or to Earth's? Most people are working on Earth and it might be more convenient for users not to have to put in a radius every time.
There was a problem hiding this comment.
I think we should keep the default as it is now (spherical Earth), but the idea is that the correct mesh is discovered when creating the FieldSet, see below
Parcels/src/parcels/_core/model.py
Line 320 in a6bb153
| if isinstance(value, SphericalMesh): | ||
| return |
There was a problem hiding this comment.
@VeckoTheGecko, can you check if this is correct behaviour for typing?
Co-authored-by: Erik van Sebille <e.vansebille@uu.nl>
Co-authored-by: Erik van Sebille <e.vansebille@uu.nl>
for more information, see https://pre-commit.ci
Co-authored-by: Erik van Sebille <e.vansebille@uu.nl>
2 participants
Description
Checklist
mainfor normal development,v3-supportfor v3 support)AI Disclosure
I used Claude to explore the repo, find the locations with hardcoded 1852 * 60 conversion factor, and suggest code snippets. I wrote the code manually (even if suggested by Claude) and made the design choices. I ran the full test suite with and without a custom radius, plus a short Mars simulation with a Mars radius.
Note: I decided to unpack the SphericalMesh object into a string to maintain the mesh == "flat"/"spherical" string comparisons throughout. This minimizes the impact of the change on the existing code. Another option would be for all mesh types to be objects with a string/flat attribute that could be checked instead, which might be useful for adding further mesh-specific properties.