-
Notifications
You must be signed in to change notification settings - Fork 193
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
Why is conformal_mapping
the last property of OrthogonalSphericalShellGrid
?
#3826
Comments
conformal_mapping
the last parameter in OrthogonalSphericalShellGrid
?conformal_mapping
the last property of OrthogonalSphericalShellGrid
?
Indeed it was added there when I was thinking that this type was going to be used for cubed sphere only. We should remove it or think about it. |
The idea was that parameter would hold the required parameters to reproduce a panel of the conformal cubed sphere, that is, the dimensions in ξ-η space and the rotation of the panel with respect to the North Pole; see
|
Ah, the property is completely essential! I'm just making a cosmetic statement about how the struct is implemented / written. |
Agreed with both, useful to distinguish between different grids and we should put it in front. |
I think I was following the parameter order also of Rectilinear and LatitudeLongitude But true, I see how it would make it very convenient to change the order, e.g., to struct OrthogonalSphericalShellGrid{Arch, C, FT, TX, TY, TZ, A, R, FR, C} <: AbstractHorizontallyCurvilinearGrid{FT, TX, TY, TZ, Arch} I can do that. |
I think
I think also the other grids would benefit from this ordering. |
For struct OrthogonalSphericalShellGrid{C, TX, TY, TZ..., Arch, FT} since then
|
Umm. We do dispatch on arch a lot Oceananigans.jl/src/DistributedComputations/distributed_grids.jl Lines 14 to 21 in 13bf409
not sure what is "convenient" about putting it at the end. |
"benefit"? Look at the silly definitions of distributed grid above. If the number of properties change (especially if it increases) the code breaks. We use the convention that the most frequently used type parameters come first. This is especially important in early stages of type design, so that changing the number of type parameters doesn't break code that requires knowledge of type parameters. I'm at a total loss to comprehend why @simone-silvestri you recommend the exact opposite convention. Maybe you should make a more detailed argument. |
Oh shoot I have completely forgot about the distributed grids |
It seems it was put here simply for "historical reasons", ie it is the most recent parameter to be added. But that is not a good way to motivate design:
Oceananigans.jl/src/Grids/orthogonal_spherical_shell_grid.jl
Line 46 in 9c6ed92
I think it belongs second, after
architecture
.Also, the type parameter should be 5th, after TZ:
Oceananigans.jl/src/Grids/orthogonal_spherical_shell_grid.jl
Line 12 in 9c6ed92
This will help with silly constructs like
https://github.com/CliMA/OrthogonalSphericalShellGrids.jl/blob/379d0de985a0927c026e6c3e43b2ebd163e054aa/src/tripolar_grid.jl#L13
cc @simone-silvestri @siddharthabishnu
The text was updated successfully, but these errors were encountered: