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

Simplifying notation for difference and interpolation operators #2236

Closed
tomchor opened this issue Feb 9, 2022 · 7 comments
Closed

Simplifying notation for difference and interpolation operators #2236

tomchor opened this issue Feb 9, 2022 · 7 comments
Labels
cleanup 🧹 Paying off technical debt

Comments

@tomchor
Copy link
Collaborator

tomchor commented Feb 9, 2022

Now that most operators are 3D we could simplify the nomenclature for the difference and interpolation operators by getting rid of the , no? I think it would make code more legible.

For example something like this (from Oceanostics)

@inline function turbulent_kinetic_energy_ccc(i, j, k, grid, u, v, w, U, V, W)
    return (ℑxᶜᵃᵃ(i, j, k, grid, ψ′², u, U) +
            ℑyᵃᶜᵃ(i, j, k, grid, ψ′², v, V) +
            ℑzᵃᵃᶜ(i, j, k, grid, ψ′², w, W)) / 2
end

could become this, which I think is way easier on the eyes:

@inline function turbulent_kinetic_energy_ccc(i, j, k, grid, u, v, w, U, V, W)
    return (ℑxᶜ(i, j, k, grid, ψ′², u, U) +
            ℑyᶜ(i, j, k, grid, ψ′², v, V) +
            ℑzᶜ(i, j, k, grid, ψ′², w, W)) / 2
end

Double interpolators could go from ℑxyᶠᶜᵃ to ℑxyᶠᶜ or ℑxᶠyᶜ.

Thoughts?

This was originally suggested by @simone-silvestri in #2214

@tomchor
Copy link
Collaborator Author

tomchor commented Feb 9, 2022

Just saw that @simone-silvestri mentioned that in #2214 (comment), my bad. I'll modify the issue to reflect that

@glwagner
Copy link
Member

glwagner commented Feb 9, 2022

I prefer the three letter notation because it's more explicit. I'm worried that mistakes might be easier to make, since the current format has a redundancy that in my experience often catches hard to find bugs when coding and formatting long expressions. I guess I also feel it helps me "visualize" terms but maybe that's just me.

Here's an example of such an expression:

@inline function norm_uᵢₐ_uⱼₐ_Σᵢⱼᶜᶜᶜ(i, j, k, grid, closure, u, v, w)
ijk = (i, j, k, grid)
uvw = (u, v, w)
ijkuvw = (i, j, k, grid, u, v, w)
uᵢ₁_uⱼ₁_Σ₁ⱼ = (
norm_Σ₁₁(ijkuvw...) * norm_∂x_u(ijk..., u)^2
+ norm_Σ₂₂(ijkuvw...) * ℑxyᶜᶜᵃ(ijk..., norm_∂x_v², uvw...)
+ norm_Σ₃₃(ijkuvw...) * ℑxzᶜᵃᶜ(ijk..., norm_∂x_w², uvw...)
+ 2 * norm_∂x_u(ijkuvw...) * ℑxyᶜᶜᵃ(ijk..., norm_∂x_v_Σ₁₂, uvw...)
+ 2 * norm_∂x_u(ijkuvw...) * ℑxzᶜᵃᶜ(ijk..., norm_∂x_w_Σ₁₃, uvw...)
+ 2 * ℑxyᶜᶜᵃ(ijk..., norm_∂x_v, uvw...) * ℑxzᶜᵃᶜ(ijk..., norm_∂x_w, uvw...)
* ℑyzᵃᶜᶜ(ijk..., norm_Σ₂₃, uvw...)
)
uᵢ₂_uⱼ₂_Σ₂ⱼ = (
+ norm_Σ₁₁(ijkuvw...) * ℑxyᶜᶜᵃ(ijk..., norm_∂y_u², uvw...)
+ norm_Σ₂₂(ijkuvw...) * norm_∂y_v(ijk..., v)^2
+ norm_Σ₃₃(ijkuvw...) * ℑyzᵃᶜᶜ(ijk..., norm_∂y_w², uvw...)
+ 2 * norm_∂y_v(ijkuvw...) * ℑxyᶜᶜᵃ(ijk..., norm_∂y_u_Σ₁₂, uvw...)
+ 2 * ℑxyᶜᶜᵃ(ijk..., norm_∂y_u, uvw...) * ℑyzᵃᶜᶜ(ijk..., norm_∂y_w, uvw...)
* ℑxzᶜᵃᶜ(ijk..., norm_Σ₁₃, uvw...)
+ 2 * norm_∂y_v(ijkuvw...) * ℑyzᵃᶜᶜ(ijk..., norm_∂y_w_Σ₂₃, uvw...)
)
uᵢ₃_uⱼ₃_Σ₃ⱼ = (
+ norm_Σ₁₁(ijkuvw...) * ℑxzᶜᵃᶜ(ijk..., norm_∂z_u², uvw...)
+ norm_Σ₂₂(ijkuvw...) * ℑyzᵃᶜᶜ(ijk..., norm_∂z_v², uvw...)
+ norm_Σ₃₃(ijkuvw...) * norm_∂z_w(ijk..., w)^2
+ 2 * ℑxzᶜᵃᶜ(ijk..., norm_∂z_u, uvw...) * ℑyzᵃᶜᶜ(ijk..., norm_∂z_v, uvw...)
* ℑxyᶜᶜᵃ(ijk..., norm_Σ₁₂, uvw...)
+ 2 * norm_∂z_w(ijkuvw...) * ℑxzᶜᵃᶜ(ijk..., norm_∂z_u_Σ₁₃, uvw...)
+ 2 * norm_∂z_w(ijkuvw...) * ℑyzᵃᶜᶜ(ijk..., norm_∂z_v_Σ₂₃, uvw...)
)
return uᵢ₁_uⱼ₁_Σ₁ⱼ + uᵢ₂_uⱼ₂_Σ₂ⱼ + uᵢ₃_uⱼ₃_Σ₃ⱼ
end

I agree though that the "minimal" notation is more readable for short expressions like the example above.

@francispoulin
Copy link
Collaborator

Even though it takes more effort, because there are three directions, I also prefer having three letters. It seems more explicit, even thought it mght be more information than is necessarily needed.

@tomchor tomchor closed this as completed Feb 9, 2022
@glwagner
Copy link
Member

@tomchor do you mind if I reopen this? I think it's worth discussing more!

@tomchor
Copy link
Collaborator Author

tomchor commented Feb 10, 2022

Not at all

@tomchor tomchor reopened this Feb 10, 2022
@simone-silvestri
Copy link
Collaborator

I am in favour of removing the indices, but not necessarily too much against keeping them either. I can see reasons for both. Since these functions are not user-facing (and would be understandable either way), I guess the best choice is to do what makes the most developers happy.

@glwagner glwagner added the cleanup 🧹 Paying off technical debt label Mar 1, 2022
@glwagner
Copy link
Member

I'm closing this issue because I'm judging that it's not of current, timely relevance to Oceananigans development. If you would like to make it a higher priority or if you think the issue was closed in error please feel free to re-open.

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

4 participants