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

Missing (-1)^{n+1} factor from hyperdiffusion term #283

Merged
merged 5 commits into from
Apr 6, 2023
Merged

Missing (-1)^{n+1} factor from hyperdiffusion term #283

merged 5 commits into from
Apr 6, 2023

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Apr 6, 2023

I believe that the hyperdiffusion term is missing a $(-1)^{n+1}$ factor? For example, for $n=1$ it's plain-old viscosity so it should be $+\nu\nabla^2\zeta$ but for $n=2$ it should be $-\nu\nabla^4\zeta$, otherwise the hyperdiffusion is not dissipative.

Am I right? If so, perhaps this mistake propagates elsewhere in the docs?

closes #286

@navidcy
Copy link
Collaborator Author

navidcy commented Apr 6, 2023

Seems like in the docs where the the eigenvalues of the Laplacian/hyperLaplacian are written down, the $(-1)^{n+1}$ factor has been taken into account.

(Btw, docs report the eigenvalues of $-\nabla^2$, right?)

@milankl
Copy link
Member

milankl commented Apr 6, 2023

This is a good point, I missed that. And it's not just in the documentation we also need to address that here

∇²ⁿ[l+1] = norm_eigenvalue^power/(3600*time_scale)*radius

Because theoretically one should be able to choose HyperDiffusion(power=3) but as it's written now it should blow up if I'm not wrong.

Update: No changes to the code needed as we normalise by the largest eigenvalue (which changes sign depending on the power!) anyway, so that the dissipative nature is retained for power=odd and even.

@milankl milankl added bug 🐞 Something isn't working documentation 📚 Improvements or additions to documentation dynamics 〰️ Affects the dynamical core labels Apr 6, 2023
@milankl milankl added this to the v0.5 milestone Apr 6, 2023
@milankl
Copy link
Member

milankl commented Apr 6, 2023

@navidcy I've changed the documentation to (hopefully) be clearer and consistent with the sign of diffusion and also adapted the dynamics to actually match the documentation. Feel free to review!

docs/src/dynamical_core.md Outdated Show resolved Hide resolved
@milankl milankl removed the bug 🐞 Something isn't working label Apr 6, 2023
Copy link
Collaborator Author

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

nice!

I can't approve because I opened the PR ;)

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@navidcy navidcy requested a review from milankl April 6, 2023 20:13
@milankl
Copy link
Member

milankl commented Apr 6, 2023

Let me quickly check that I can reproduce the figures in #286 with the signs changed in the dynamical core

@milankl milankl merged commit 2c89735 into SpeedyWeather:main Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📚 Improvements or additions to documentation dynamics 〰️ Affects the dynamical core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing (-1)^{n+1} factor from hyperdiffusion term
2 participants