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

Improving tracer budget tests #942

Closed
glwagner opened this issue Sep 14, 2020 · 4 comments
Closed

Improving tracer budget tests #942

glwagner opened this issue Sep 14, 2020 · 4 comments
Labels
testing 🧪 Tests get priority in case of emergency evacuation

Comments

@glwagner
Copy link
Member

glwagner commented Sep 14, 2020

In test_dynamics.jl we test budgets for tracers and momentum variables:

function test_isotropic_diffusion_budget(fieldname, model)
set!(model; u=0, v=0, w=0, T=0, S=0)
set!(model; Dict(fieldname => (x, y, z) -> rand())...)
field = get_model_field(fieldname, model)
return test_diffusion_budget(fieldname, field, model, model.closure.ν, model.grid.Δz)
end
function test_biharmonic_diffusion_budget(fieldname, model)
set!(model; u=0, v=0, w=0, T=0, S=0)
set!(model; Dict(fieldname => (x, y, z) -> rand())...)
field = get_model_field(fieldname, model)
return test_diffusion_budget(fieldname, field, model, model.closure.νz, model.grid.Δz, 4)
end

However, the test is imperfect because 1) it does not test velocity components in non-periodic directions and 2) it uses a too-loose tolerance.

We can improve this test by tapering the initial condition for the field in question to zero for velocity fields in bounded directions.

EDIT: a little thought goes a long way: in Bounded directions, momentum is not conserved in general unless pressure at the boundaries is zero. Perhaps we should be happy just to test momentum conservation in periodic directions. This is still an important test that ensures the no flux condition is correctly implemented.

Also, we can make the tolerance more strict. Finally, once this test works well, we can get rid of the tracer_conserved_in_channel test in test_time_stepping.jl.

@glwagner
Copy link
Member Author

We probably should run these tests with different advection schemes in addition to different diffusion schemes.

cc @navidcy

@ali-ramadhan
Copy link
Member

@glwagner Does your still-open PR #1486 resolve this issue?

@glwagner
Copy link
Member Author

glwagner commented Mar 17, 2021

I don't think it should because the issue here is whether we conserve tracers when we have explicit diffusion and should probably use no flux boundary conditions. The tests on PR #1486 test whether fluxes across boundaries are correctly prescribed (and uses closure=nothing).

The tests are pretty similar though and resolving this issue is a priority now. We also need to extend the tests mentioned in this issue to other models and grids.

@navidcy navidcy added the testing 🧪 Tests get priority in case of emergency evacuation label Jul 3, 2021
@glwagner
Copy link
Member Author

I'm closing this issue because I'm judging that it's not of current, timely relevance to Oceananigans development. If you would like to make it a higher priority or if you think the issue was closed in error please feel free to re-open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing 🧪 Tests get priority in case of emergency evacuation
Projects
None yet
Development

No branches or pull requests

3 participants