Skip to content

Changes default AMD poincare coefficient #4494

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

Merged
merged 8 commits into from
May 20, 2025
Merged

Conversation

jagoosw
Copy link
Collaborator

@jagoosw jagoosw commented May 9, 2025

Following discussion in #4367

@jagoosw jagoosw added the numerics 🧮 So things don't blow up and boil the lobsters alive label May 9, 2025
@glwagner
Copy link
Member

glwagner commented May 9, 2025

You'll have to manually change the coefficient in the regression tests (once those pass we can merge right away).

@glwagner
Copy link
Member

glwagner commented May 9, 2025

made the necessary change

@glwagner
Copy link
Member

glwagner commented May 9, 2025

If you want extra credit, you can expand the discussion in the docstring. You could link the relevant issue or just write a simple independent explanation.

@navidcy
Copy link
Member

navidcy commented May 11, 2025

I think the discussion in #4367 worths some mention in the docstring. Even if it's just a short comment + pointing people to the PR discussion?

@glwagner
Copy link
Member

@jagoosw, can you update the docstring to explain why we chose 1/3 vs 1/12 with appropriate references?

@jagoosw
Copy link
Collaborator Author

jagoosw commented May 12, 2025

I'm not sure what to put since the references already there support 1/3 just the value was wrong?

Comment on lines 66 to 67
By default: `C = Cν = Cκ = 1/3`, which is appropriate for a finite-volume method employing a
second-order advection scheme, and `Cb = nothing`, which turns off the buoyancy modification term.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagoosw please add text here from the issue, with appropriate citations, that explain why 1/3 is the right choice

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also put in a reference to #4367 which has some relevant plots

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The discussion will be an elaboration of this remark:

"I think they're getting there a different way but that in equation 39 they're finding the equivalent of setting it to 1/3?"

Comment on lines +73 to +83
lines!(ax, [exp(1), exp(3)], x->(x/exp(1))^(-5/3)*exp(15), color = :red, linestyle = :dash)

text!(ax, exp(0.8), exp(13); text = L"$E(k)\sim k^{-5/3}$", color = :white)

Colorbar(fig[1, 2], colormap = :oslo, colorrange = (0, 10), label = "Time (s)")

k_filt = 1/sqrt(closure.Cν * 3 / (1/(2*minimum_xspacing(grid))^2 + 1/(2*minimum_yspacing(grid))^2 + 1/(2*minimum_zspacing(grid))^2))

lines!(ax, ones(2) .* k_filt, [exp(0), exp(10)], color = :red, linestyle = :dash)

text!(ax, k_filt, exp(10); text = "1/δ")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lines!(ax, [exp(1), exp(3)], x->(x/exp(1))^(-5/3)*exp(15), color = :red, linestyle = :dash)
text!(ax, exp(0.8), exp(13); text = L"$E(k)\sim k^{-5/3}$", color = :white)
Colorbar(fig[1, 2], colormap = :oslo, colorrange = (0, 10), label = "Time (s)")
k_filt = 1/sqrt(closure.* 3 / (1/(2*minimum_xspacing(grid))^2 + 1/(2*minimum_yspacing(grid))^2 + 1/(2*minimum_zspacing(grid))^2))
lines!(ax, ones(2) .* k_filt, [exp(0), exp(10)], color = :red, linestyle = :dash)
text!(ax, k_filt, exp(10); text = "1/δ")
lines!(ax, [exp(1), exp(3)], x->(x/exp(1))^(-5/3)*exp(15), color = :red, linestyle = :dash)
text!(ax, exp(0.8), exp(13); text = L"$E(k)\sim k^{-5/3}$", color = :white)
Colorbar(fig[1, 2], colormap = :oslo, colorrange = (0, 10), label = "Time (s)")
k_filt = 1/sqrt(closure.* 3 / (1/(2*minimum_xspacing(grid))^2 + 1/(2*minimum_yspacing(grid))^2 + 1/(2*minimum_zspacing(grid))^2))
lines!(ax, ones(2) .* k_filt, [exp(0), exp(10)], color = :red, linestyle = :dash)
text!(ax, k_filt, exp(10); text = "1/δ")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jagoosw can you address this?

@glwagner glwagner merged commit 925f4c9 into main May 20, 2025
56 of 58 checks passed
@glwagner glwagner deleted the jsw/amd-default-coefficient branch May 20, 2025 04:02
@tomchor
Copy link
Collaborator

tomchor commented May 20, 2025

Sorry I'm a bit late here. I understand why these changes were made. I'm still a bit confused though: what should this coefficient be for a WENO of order $n$ advection scheme? Do we know?

@glwagner
Copy link
Member

No, we don't know. Although I think that combination exhibits the lowest effective resolution (thus increasing computational expense) compared to 2nd order advection + AMD, or WENO only. @xkykai and @simone-silvestri might know more.

@xkykai
Copy link
Collaborator

xkykai commented May 20, 2025

No, we don't know. Although I think that combination exhibits the lowest effective resolution (thus increasing computational expense) compared to 2nd order advection + AMD, or WENO only. @xkykai and @simone-silvestri might know more.

Indeed. AMD + WENO seems to be the most dissipative when compared with 2nd order + AMD and WENO + no SGS closure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
numerics 🧮 So things don't blow up and boil the lobsters alive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants