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

Add finite depth parameter to SQG model (surfaceqg.jl) #374

Open
mncrowe opened this issue Dec 6, 2024 · 7 comments · May be fixed by #375
Open

Add finite depth parameter to SQG model (surfaceqg.jl) #374

mncrowe opened this issue Dec 6, 2024 · 7 comments · May be fixed by #375
Labels
🤥 enhancement New feature or request science 🔬

Comments

@mncrowe
Copy link
Contributor

mncrowe commented Dec 6, 2024

I suggest adding a new depth parameter, say H, which can either be $\infty$, giving the current model, or finite.

In the finite case, the only change is that the Fourier space operator linking b and ψ changes from

$$ \hat{b} = |\textbf{k}|\hat{\psi}, $$

to

$$ \hat{b} = |\textbf{k}| \tanh(|\textbf{k}| H)\hat{\psi}. $$

From looking at the code, I think this just means changing the nonlinear function calcN_advection!, though some of the diagnostics might also need minor changes depending on how they're calculated.

Happy to help implement this.

@glwagner glwagner added the 🤥 enhancement New feature or request label Dec 6, 2024
@glwagner
Copy link
Member

glwagner commented Dec 6, 2024

Nice! It might make sense to build this feature as an option to precompute this function and store it in an array. That would be more flexible and also may save computational cost since tanh are relatively expensive.

@mncrowe
Copy link
Contributor Author

mncrowe commented Jan 21, 2025

I've made a working version of this here. A few notes:

  • This probably isn't how you want to handle types in Params. I'm guessing the way to go is similar to SingleLayerQG and have different structures for the finite and infinite parameter cases. I've also fudged the array type slightly in a way that isn't consistent with what you've got. I can sort this out if you let me how how you want to proceed with this.
  • I haven't implemented any testing or examples currently.
  • There doesn't seem to be any speed difference between the finite and infinite H cases with this implementation.
  • No pull request currently, will do so once I'm a bit closer to a final version.

@navidcy
Copy link
Member

navidcy commented Jan 21, 2025

Cool! It's much easier to see the differences you suggested once you open a PR.

But yes, the if-else statement

https://github.com/mncrowe/GeophysicalFlows.jl/blob/22e2bd4aad11647360ae02d79966bb2caee9aefe/src/surfaceqg.jl#L373-L377

should be replaced with multiple dispatch on the type of Params.

@mncrowe
Copy link
Contributor Author

mncrowe commented Jan 21, 2025

PR here.

@mncrowe mncrowe linked a pull request Jan 21, 2025 that will close this issue
@navidcy
Copy link
Member

navidcy commented Jan 22, 2025

Can we combine the two definitions (finite and infinite depth) given that:

julia> tanh(Inf)
1.0

?

What if there is a depth parameter with default value Inf?

@mncrowe
Copy link
Contributor Author

mncrowe commented Jan 22, 2025

Can we combine the two definitions (finite and infinite depth) given that:

julia> tanh(Inf)
1.0
?

Yes, I think I've tried this and the only issue is that for the k = 0 mode, you get tanh(0 * Inf) so you have to set that mode manually.

What if there is a depth parameter with default value Inf?

My thinking was that you should only save the values of DirNeu if it was slow to calculate, as Greg hinted might be the case for tanh. Hence the two cases being separated. If you think this isn't necessary, we can revert and either pre-compute or calculate DirNeu? Though the option to pre-save might be useful?

@navidcy
Copy link
Member

navidcy commented Jan 23, 2025

Fair point about tanh being slow; let's keep it as is.

@navidcy navidcy linked a pull request Jan 23, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤥 enhancement New feature or request science 🔬
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants