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

Mistake in stretching matrix in multilayerqg.jl #339

Closed
mpudig opened this issue Nov 2, 2023 · 6 comments · Fixed by #343
Closed

Mistake in stretching matrix in multilayerqg.jl #339

mpudig opened this issue Nov 2, 2023 · 6 comments · Fixed by #343
Labels
❓ question Further information is requested science 🔬

Comments

@mpudig
Copy link
Collaborator

mpudig commented Nov 2, 2023

Hi,

We (Shafer Smith's group at NYU) noticed a possible typo in how the reduced gravities are defined in multilayerqg (https://github.com/FourierFlows/GeophysicalFlows.jl/blob/main/src/multilayerqg.jl#L355).

It is our understanding that in the multi-layer QG equations, one should divide by a reference density in the reduced gravities (see Vallis, 2017 p.186), rather than by the density of each layer as is currently done in multilayerqg (as well as in its documentation).

@navidcy, could you comment on whether this is in fact a typo or if there was a reason behind this choice for the reduced gravities?

Thanks!

@navidcy
Copy link
Member

navidcy commented Nov 2, 2023

Hm... trying to remember.

Does the division with reference density implies that the Boussinesq approximation holds?

@navidcy navidcy added ❓ question Further information is requested science 🔬 labels Nov 2, 2023
@navidcy
Copy link
Member

navidcy commented Nov 2, 2023

The discussion #325 is probably relevant. The discussion was pointing out two things: a sign error (we fixed in #329) but also pointing out that having the denominator in the reduced gravities vary with layer brings problems in setting up an Eady problem.

We should resume the discussion. Perhaps we close this issue and continue chatting in that discussion and when we converge on what to do we should open a PR and fix it?

@mpudig
Copy link
Collaborator Author

mpudig commented Nov 2, 2023

Sorry @navidcy, I hadn't seen this extensive discussion! We originally picked up this mistake in pyqg as Matt Lobo seems to have as well - ha! Happy to chat on that discussion. I believe that yes the reference density comes from Boussinesq.

@navidcy
Copy link
Member

navidcy commented Nov 2, 2023

No worries!

@mpudig mpudig closed this as completed Nov 3, 2023
@navidcy
Copy link
Member

navidcy commented Nov 15, 2023

hi @mpudig, don't we still need to resolve whether we divide with $\rho_1$ or $\rho_n$?

@mpudig
Copy link
Collaborator Author

mpudig commented Nov 20, 2023

Hi @navidcy, sorry for the slow uptake!

Is the conclusion from discussion #325 that we should do away with accepting $\rho$ as an input field for multilayerqg and only accept buoyancy? That way, if the stratification is constant then the reduced gravity will be constant layer-to-layer as we would expect.

multilayerqg will then not need any definition of $g$ or $\rho$ (nor a reference density) and will just require a buoyancy field input, $b$. I have had a tentative go at this change (see my PR).

If instead we do want to keep $\rho$ as an input, then yes I think we should simply divide by $\rho_1$ (the first layer value) to form the reduced gravity array (in order to adhere to the Boussinesq approximation and to avoid spurious internal PV gradients as Matt Lobo noted in the discussion).

Let me know what you think and happy to make changes as you see fit!

@navidcy navidcy linked a pull request Dec 19, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
❓ question Further information is requested science 🔬
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants