Skip to content
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

Open
glwagner opened this issue Oct 5, 2024 · 10 comments
Open
Labels
cleanup 🧹 Paying off technical debt

Comments

@glwagner
Copy link
Member

glwagner commented Oct 5, 2024

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:

I think it belongs second, after architecture.

Also, the type parameter should be 5th, after TZ:

struct OrthogonalSphericalShellGrid{FT, TX, TY, TZ, A, R, FR, C, Arch} <: AbstractHorizontallyCurvilinearGrid{FT, TX, TY, TZ, Arch}

This will help with silly constructs like

https://github.com/CliMA/OrthogonalSphericalShellGrids.jl/blob/379d0de985a0927c026e6c3e43b2ebd163e054aa/src/tripolar_grid.jl#L13

cc @simone-silvestri @siddharthabishnu

@glwagner glwagner added the cleanup 🧹 Paying off technical debt label Oct 5, 2024
@glwagner glwagner changed the title Why is conformal_mapping the last parameter in OrthogonalSphericalShellGrid? Why is conformal_mapping the last property of OrthogonalSphericalShellGrid? Oct 5, 2024
@navidcy
Copy link
Collaborator

navidcy commented Oct 5, 2024

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.

@navidcy
Copy link
Collaborator

navidcy commented Oct 5, 2024

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

conformal_mapping = (; ξ, η, rotation)

@glwagner
Copy link
Member Author

glwagner commented Oct 6, 2024

Ah, the property is completely essential! I'm just making a cosmetic statement about how the struct is implemented / written.

@simone-silvestri
Copy link
Collaborator

Agreed with both, useful to distinguish between different grids and we should put it in front.

@navidcy
Copy link
Collaborator

navidcy commented Oct 8, 2024

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.

@simone-silvestri
Copy link
Collaborator

I think Arch at the end is quite convenient. We never dispatch on Arch. Also TX, TY, TZ are quite useful in front. Most of the dispatch is done on the topology. I am not sure about FT in front. FT is never used for dispatch.
The most convenient ordering of parameters in my opinion would be

struct OrthogonalSphericalShellGrid{TX, TY, TZ, C..., Arch, FT}

I think also the other grids would benefit from this ordering.

@simone-silvestri
Copy link
Collaborator

For OrthogonalSphericalShellGrid specifically we could do

struct OrthogonalSphericalShellGrid{C, TX, TY, TZ..., Arch, FT}

since then C is removed when specializing the different grids, for example:

const CubedSpherePanelGrid{TX, TY, TZ...} = OrthogonalSphericalShellGrid{<:CubedSphereMapping, TX, TY, TZ...} where {TX, TY, TZ...}

@glwagner
Copy link
Member Author

glwagner commented Oct 8, 2024

Umm. We do dispatch on arch a lot

const DistributedGrid{FT, TX, TY, TZ} = AbstractGrid{FT, TX, TY, TZ, <:Distributed}
const DistributedRectilinearGrid{FT, TX, TY, TZ, FX, FY, FZ, VX, VY, VZ} =
RectilinearGrid{FT, TX, TY, TZ, FX, FY, FZ, VX, VY, VZ, <:Distributed} where {FT, TX, TY, TZ, FX, FY, FZ, VX, VY, VZ}
const DistributedLatitudeLongitudeGrid{FT, TX, TY, TZ, M, MY, FX, FY, FZ, VX, VY, VZ} =
LatitudeLongitudeGrid{FT, TX, TY, TZ, M, MY, FX, FY, FZ, VX, VY, VZ, <:Distributed} where {FT, TX, TY, TZ, M, MY, FX, FY, FZ, VX, VY, VZ}

not sure what is "convenient" about putting it at the end.

@glwagner
Copy link
Member Author

glwagner commented Oct 8, 2024

struct OrthogonalSphericalShellGrid{TX, TY, TZ, C..., Arch, FT}

I think also the other grids would benefit from this ordering.

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

@simone-silvestri
Copy link
Collaborator

Oh shoot I have completely forgot about the distributed grids

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup 🧹 Paying off technical debt
Projects
None yet
Development

No branches or pull requests

3 participants