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

Option to specify topographic PV large-scale gradients in MultiLayerQG #313

Merged
merged 12 commits into from
Dec 22, 2022

Conversation

apaloczy
Copy link
Collaborator

@apaloczy apaloczy commented Dec 9, 2022

This closes #307 by adding options to specify a nonperiodic component of the topographic PV gradients (for example a planar slope) in the MultiLayerQG module. Also added one test.

The gradients of the nonperiodic part of the topographic PV are now specified separately via etax_nonperiodic and etay_nonperiodic in MultiLayerQG.Problem, and are added to the gradients of the periodic part of the topographic PV (calculated via FFTs).

@apaloczy apaloczy requested a review from navidcy December 9, 2022 15:42
src/multilayerqg.jl Outdated Show resolved Hide resolved
src/multilayerqg.jl Outdated Show resolved Hide resolved
src/multilayerqg.jl Outdated Show resolved Hide resolved
@navidcy
Copy link
Member

navidcy commented Dec 18, 2022

Sorry for slacking on this. I’m on holiday.

@apaloczy
Copy link
Collaborator Author

No worries at all! Take your time.

@navidcy
Copy link
Member

navidcy commented Dec 18, 2022

@apaloczy, I think it's good to go! Have a look at the docstrings additions I made and modify if needed.

@navidcy
Copy link
Member

navidcy commented Dec 18, 2022

I believe I gave you write rights so you can merge.

@navidcy navidcy added 🚰 user experience Make it easier for users 🤥 enhancement New feature or request labels Dec 18, 2022
@navidcy
Copy link
Member

navidcy commented Dec 19, 2022

@apaloczy, I'm wondering whether there is a requirement that etax_nonperiodic and etay_nonperiodic are themselves periodic! I think they should! Otherwise, e.g., the Qx term won't be periodic and then errors will be introduced via

https://github.com/apaloczy/GeophysicalFlows.jl/blob/f51365a4587fd483bb7231418e26b3ec6d81bc66/src/multilayerqg.jl#L774-776

right?

@navidcy navidcy self-requested a review December 19, 2022 00:25
Copy link
Member

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

let's think more whether the gradients of the non-periodic component of the PV have to be periodic

@navidcy
Copy link
Member

navidcy commented Dec 19, 2022

Giving this more thought, I doubt the non-periodic topo PV would work with anything but constant slope η = a x + b y.

@glwagner
Copy link
Member

The equations themselves have to be periodic; otherwise the discontinuity at domain "edges" will cause simulations to blow up.

@navidcy
Copy link
Member

navidcy commented Dec 21, 2022

@apaloczy I suggest we change etax_nonperiodic to topographic_pv_x_slope and let that be a Float. Same for y. And clarify that users can prescribe large-scale topographic PV gradient in each direction.

What do you think?

@navidcy
Copy link
Member

navidcy commented Dec 21, 2022

I had a go at it. Have a look. :)

@navidcy navidcy changed the title Option to specify nonperiodic parts of the topographic PV gradients in MultiLayerQG Option to specify topographic PV large-scale gradients in MultiLayerQG Dec 21, 2022
@navidcy navidcy self-requested a review December 22, 2022 11:28
@apaloczy
Copy link
Collaborator Author

Sorry for the slowness here. I was offline for a couple of days myself. This all sounds great, I think having the large-scale topographic PV slopes as floats makes sense.

It also makes sense that Qx and Qy have to be periodic otherwise boundary errors will appear, and a planar slope is the only non-periodic topographic PV possible since it gives a periodic, constant PV gradient.

@apaloczy apaloczy merged commit 386fd9a into FourierFlows:main Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤥 enhancement New feature or request 🚰 user experience Make it easier for users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to specify PV gradients explicitly rather than computing them spectrally
3 participants