-
Notifications
You must be signed in to change notification settings - Fork 193
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
ShallowWaterModel.bathymetry
?
#1712
Comments
This still needs to be done. I will raise it on list of priorities of my to-do list. It will be very easy when it's done. |
Just found this issue while setting up a |
I think that's right --- it's essentially a matter of applying the transformation https://clima.github.io/OceananigansDocumentation/stable/physics/shallow_water_model/ ? I think this might be achieved two ways:
total_depth = solution.h + bathymetry in the model constructor and add this property to As for how to handle the bathymetry I think something similar to what we do for immersed boundaries would be nice:
eg, in the constructor for if !isnothing(bathymetry) # user has provided non-default bathymetry
bathymetry_field = Field{Center, Center, Nothing}(grid)
set!(bathymetry_field, bathymetry) # works for functions and arrays
total_depth = solution.h + bathymetry_field
else # there's no bathymetry
total_depth = solution.h
end
# Later, make sure to use `bathymetry_field` when instantiating `ShallowWaterModel`. @apaloczy that would be pretty awesome if you want to tackle this! Happy to provide help along the way if you open a PR when you start down this road. I can also help with docs. |
Thanks @glwagner and @francispoulin, I will give this a try and ping you both along the way. @francispoulin, I feel that having the free surface elevation rather than total height would be preferable too. About option 2, I am missing how introducing the h -> h + b transformation would work for the pressure gradient term. The governing equations only apply for flat-bottom cases the way they are currently set up, right? I ask this because the total thickness h = H + 𝜂 - b is used instead of the free surface 𝜂 in the pressure gradient terms and -g∂ₓ𝜂 = -g∂ₓ(h + b - H) = -g∂ₓh only if b = 0. Here H, 𝜂 and b are the mean thickness, the free surface and the bathymetry, respectively. So with bathymetry we would have -g∂ₓ(h + b - H) = -g∂ₓh -g∂ₓb, and in conservative form, -g∂ₓ(h²/2) -gh∂ₓb, right? so we would end up with this extra term proportional to the local bottom slope (as in e.g., FourierFlows/GeophysicalFlows.jl#163 (comment)). |
Your are correct @apaloczy that we need to be careful. One thing I should point out is that we are solving the equations in conservation form, so it is a bit differente than the link you cited. If you look at equations 7 to 9 in this paper then you will see how this is written in conservation form. Note that the syntax is Also, this paper simulates the instabilty of a geostrophically balanced Bickley jet over sloping topogrpahy. We could take the |
I think the main reason to use total height is that it generalizes to a "stacked" shallow water model (where there are N layers rather than just 1). But perhaps the choice depends on whether shallow water model is valuable mostly as a stand-alone model for physics problems or whether it's intent is more as a testbed for develping numerical methods, etc (with perhaps the eventual possibility of generalizing to N layers). |
It's probably easier to keep things in terms of |
Right. Another reason to keep |
Oh, this seems right. Apologies for the confusion. I'm still not sure we want to analytically integrate one of the pressure gradient terms
It might be worth testing both possibilities to see if one has favorable numerical properties? Perhaps this is related to the whole issue re: well-balanced methods, etc... ? |
I agree with @glwagner . Currently, One issue is that since we are using a finite volume method, the above term presents a bit of a problem as it's not in divergence form However, even in the other formulation, as you can see from the paper cited above, we still end up getting a term that is not in divergence form, I support trying @glwagner 's proposal of keeping the pressure term as he wrote it above. |
We should probably also address #1866... |
Yes, most definitely! |
So it seems the best way forward for implementing bathymetry is to do it in the non-conservative form of the equations after #2522 is merged. Do we then want to have the conservative form of |
That seems like a good starting point to me. Do the easy thing and start running simulations! |
I agree whole heartedly with @glwagner . Let's get the easiest thing working first and then we can build from there. |
The bathymetric term is already in #2522 |
Is this just a placeholder? Or does indeed
ShallowWaterModel.bathymetry
supports non-trivial bathymetry?The text was updated successfully, but these errors were encountered: