-
Notifications
You must be signed in to change notification settings - Fork 190
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
Include background tracer and velocities in closure tendency terms #3646
base: main
Are you sure you want to change the base?
Conversation
Probably before merging this we also want to use the total velocities in the stress computation |
But I wonder also if we should add a feature to |
…liMA/Oceananigans.jl into glw/background-flux-divergence
Ok @hdrake @liuchihl I have added the option to include or not the background field when computing closure fluxes. They are noit included by default. If you want to include them you need to build the background_fields = BackgroundFields(; background_closure_fluxes=true, b=B) where Let me know if this seems like a good interface and also if it works. |
Looks like a good interface to me. But is it on purpose that there is only support for background fields in the @liuchihl will test it in our configurations. |
Well yes, it's substantial effort to support background fields. So we implemented it in the nonhydrostatic model first. Nobody has requested having background fields for the hydrostatic model. It's not impossible but might require some thinking if it's going to work with the more complicated turbulence closures (like CATKE or k-epsilon) that sometimes get used for hydrostatic applications. Since the nonhydrostatic model is fast (at least on one GPU) the hydrostatic model is mostly important for simulations on the sphere (although this statement needs to be evaluated more carefully for complex domains when we have a proper nonhydrostatic solver). |
@glwagner, can you help us understand how the diffusive flux calculations here interact with gradient boundary conditions? We've run some simulations and seem to be getting the expected behavior in the interior, where buoyancy tendencies are due to the convergence of both the perturbation and background diffusive fluxes. However, we are not seeing the expected behavior from the flux convergence due to boundary conditions (on the domain, not even considering immersed boundary conditions yet). My expectation was that the
which in our case should be the sum of the perturbation and background tracer fields. Instead, it seems that our solutions are behaving as though the |
Oceananigans.jl/src/ImmersedBoundaries/immersed_boundary_condition.jl Lines 100 to 101 in 738d172
Contrast this with the routine for Oceananigans.jl/src/ImmersedBoundaries/immersed_boundary_condition.jl Lines 103 to 107 in 738d172
These gradients are then used to compute the flux, for example Oceananigans.jl/src/ImmersedBoundaries/immersed_boundary_condition.jl Lines 117 to 122 in 738d172
I don't know exactly what it means for a gradient to be applied to the field. Can you please clarify? |
Additionally can you help me understand why you are using |
@glwagner Thanks for implementing the total tracer diffusive flux at a high level. After running several tests, I found it to work exceptionally well! I conducted a series of tests: 1) comparing 1D vs 3D, 2) with and without the Coriolis force, and 3) with and without the immersed boundary. Everything looks great! Here are some simple examples on a rotated coordinate:
nonconstantdiffusivity250days-theta.0.002_Nx4_Ny4_smallf_zlargerf.mp4
nonconstantdiffusivity8days-theta.0.2_Nx4_Ny4_immersed_3Dfields_withcrossflux.mp4The only caveat mentioned by @hdrake is that |
Flux boundary conditions are both conceptually and practically easier to implement when the boundary fluxes are zero or constant. They can be trickier when they depend on interior flow variables. In our case, for example, the boundary condition on the perturbation variable Imposing a flux boundary condition requires knowing the diffusivity |
I just meant where in the code the gradient boundary conditions get imposed, which you've shown us is in the calculation of the gradients that feed into the downgradient diffusive fluxes that are used in the diffusive flux divergence contribution to the tracer tendencies. Thanks! |
Okay great. I would only add, I think it's clearer to think of the gradient as being used to diagnose the cross-boundary flux (rather than imposed). I guess the point here is that there is actually an apparent flux of tracer into the perturbation field because of the presence of the background. So we are imagining that the background is being maintained by some large scale circulation which is ultimately the source of this apparent flux. |
Thanks @liuchihl ! |
Hi @hdrake @liuchihl please review this PR and let me know if things look good. I've tried to consistently add the background fields to the fields that go into computing both tracer fluxes and subgrid stresses. It makes things a little more complicated, but the code is more general now. I think with this change, things like a background shear will be used in computing a subgrid stress divergence. But the shear is still not used when computing a nonlinear diffusivity (this could be added though...) |
Thanks @glwagner! We haven't been using background velocities in our setups, so won't have as much to say on that yet. But we'll continue testing the background tracer fluxes. @samlewin, are you using background shear in addition to background tracer fields in your configurations, or is the shear just in your initial conditions? This PR might be relevant. |
What do you mean by nonlinear diffusivity here? |
For example the Smagorinsky-Lilly diffusivity |
Just need an approval and then we can merge. |
Is the new feature tested? If so I'm very happy to approve... I couldn't see that from the File changes but if you can point me to it it'd be nice thanks! |
Yeah, I've done some tests here: #3646 (comment) |
@liuchihl, I think @navidcy is asking about smaller unit tests that could be run in Continuous Integration (CI) to automatically test the package whenever any changes get made. The simplest test would be a 1D simulation with a background linear tracer profile with boundary conditions of |
I was referring to tests in the CI suite, not tests that somebody run somewhere once with one version of the code... (the latter is GREAT, not to diminish the effort @liuchihl!! test like those help us understand what was wrong and how to fix it... but they will get forgotten and not necessarily be repeated every time we update the code .... and if a new bug comes along we might not be able to catch it quickly if there is nothing in the CI suite) |
Should we add the test @hdrake suggests in this PR or leave it for later? Happy either way... I will elaborate a bit my train of thoughts and where my question came before. I opened the PR in an attempt to review. I looked at the file changes, I like the new feature... if I had seen that some test was added in the CI to test the new feature and the test suite was passing I'd feel very comfortable with it and would be more keen to approve. Now that I don't see that I need to look into the details of what happened here before I feel comfortable to approve (which I'm happy to do, but perhaps on Fri or next week) |
+1 for CI tests in this PR! Also, can someone please update the first comment with a brief example/description of the usage? I'm having a bit of trouble visualizing it just from the source code. |
agreed for the test. The rest looks good |
cc @hdrake @liuchihl