Add finite depth functionality to SurfaceQG
#375
Conversation
|
I've added multiple dispatch to replace the if-else logic. CI failing for documentation, not sure why. |
There was a problem hiding this comment.
Suggestion:
At the moment there is no variable for stream function. If we introduce ψh in Vars then we can call streamfunctionfromb! within calcN_advection!. This way, by having multiple dispatch on streamfunctionfromb! we'll sort out the finite/infinite depth cases without having to almost-duplicate calcN_advection!. What do you think?
|
@mncrowe I tried to simplify. What do you think? |
| @@ -4,6 +4,7 @@ export | |||
| Problem, | |||
| set_b!, | |||
| updatevars!, | |||
| streamfunctionfromb!, | |||
There was a problem hiding this comment.
In the end this is not used, right? Delete?
There was a problem hiding this comment.
It's not used. However, it might still be useful as the streamfunction is not available in vars and often it's very useful to know. Removing it would be ok as we have the inversion relation saved, though it's perhaps more user-friendly to have a function that calculates it rather than expecting users to do the Fourier transforms themselves?
There was a problem hiding this comment.
Note that "exporting" is really about choosing a stable user-API (not necessarily defining all functions that users need). Its ok to require users to qualify their names or write using GeophysicalFlows: streamfunctionfromb!. Not expressing an opinion about exporting or not here, just want to make sure that the goals are clear.
There was a problem hiding this comment.
Yeah, this reply was intended to answer this and the question below in one go so I didn't separate removing the function and removing the export. I'll leave the choice of exporting vs not to you but I think including it (or perhaps a non-! version) would be useful.
There was a problem hiding this comment.
ah I was confused about @navidcy's suggestion
src/surfaceqg.jl
Outdated
| """ | ||
| streamfunctionfromb!(ψ, bh, params, grid) | ||
| streamfunctionfromb!(ψ, prob) | ||
|
|
||
| Compute the streamfunction `ψ` from the buoyancy `bh` in Fourier space. | ||
| (Note that `bh = prob.sol`.) | ||
| """ | ||
| function streamfunctionfromb!(ψ, bh, params, grid) | ||
| ldiv!(ψ, grid.rfftplan, @. params.ψhfrombh * bh) | ||
| return nothing | ||
| end |
There was a problem hiding this comment.
Since the method anyway allocates memory, wouldn't it be simpler if it was something like:
function get_streamfunction(prob)
ψh = @. prob.params.ψhfrombh * prob.sol
return irfft(ψh, prob.grid.nx)
endThere was a problem hiding this comment.
Yes, looks good, this is what I was getting at with 'non-! version' above.
Also, is there a reason (other than the !) why rfft and irfft are used in some places and mul! and ldiv! in others?
There was a problem hiding this comment.
We use ldiv! and mul! to avoid allocations when already the arrays we’ll put the values in are pre-existing.
|
This is ready if we add some tests. Also if you wanna include the new diagnostic perhaps also a test for that? |
Will add the diagnostic next week and think about some tests for that. A simple way to test the finite depth bit would be to just create a loop on |
|
We should include some finite-depth info at the docs module page: https://github.com/mncrowe/GeophysicalFlows.jl/blob/main/docs/src/modules/surfaceqg.md |
I agree, I'll type out some stuff when I get a chance |
Addresses issue 374