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 unit tests for background field flux divergence #4027

Conversation

liuchihl
Copy link
Contributor

@liuchihl liuchihl commented Jan 3, 2025

This PR adds a unit test for #3646

The tests compare model results with and without background fields using a linear stratification profile.

glwagner and others added 24 commits November 15, 2024 08:33
* Simplify interpolation for triply-flat grids

* Generalize flatten_node

* Bump version
* leftover

* fix step free surface

* wrong PR
* reduce parameter space

* change other kernels for symmetry

* only free surface forcing

* pass specific forcing in kernels

* bugfix

* bugfix

* reduce parameter space

* import on_architecture

* some changes

* put model_fields before the tracer loop

* remove tracers from here

* remove auxiliary fields

* revert to previous code
* add this correction

* add a rotation operator

* add test

* revert

* update rotation

* fix tests

* actually tests are ok

* is this the correct direction?

* better vector rotation

* better rotation

* fix tests

* remove test for the moment

* bump minor version
* this should work

* remove all the @show

* restart tests

* will this work?

* test more than one position

* bugfix

* this should work

* remove unused truncate

* new interpolate

* I think this works

* try option 2

* no need for flip anymore

* fix gpu tests

* change comment and restore docstring

* fix the interpolate

* add (nothing nothing nothing) interpolate

* do not reuse names

* bugfix

* change ᵃ to ᶜ

* back to previous code

* add the :a in the metrics
* new bottom height

* now it should work

* comment

* comment

* remove circular dependency for now

* some bugfixes

* change name to column_height

* correct column height

* whoops

* another correction

* some more changes

* another correction

* couple of more bugfixes

* more bugfixes

* this should make it work

* unify the formulations

* correct implementation

* correct implementation

* correct partial cell bottom

* use center immersed condition for grid fitted boundary

* use the *correct* center node

* no h for z-values!

* simplify partial cells

* make sure we don't go out of bounds

* back to immersed condition

* name changes

* bugfix

* another bugfix

* fix bugs

* some corrections

* another bugfix

* domain_depth

* some remaining `z_bottom`s

* back as it was

* bugfix

* correct correction

* static_column_depth

* Update src/Grids/grid_utils.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* address comments

* new comment

* another name change

* AGFBIBG istead of AGFBIB and z_bottom only in TurbulenceClosures

* some bugfixes

* better definition of bottom height

* fixed partial cell

* fixed partial cells

* remove OrthogonalSphericalShellGrids while we decide what to do

* these files shouldn't go here

* give it a try

* runs

* small test

* test it in a more serious simulation

* works!

* remove the show

* remove useless terms

* add some validation

* some small changes

* new operators

* this was removed

* start refactoring

* start refactoring

* some more refactoring

* big overhaul

* more refactoring

* compiles

* more refactoring...

* correction

* use a module for now

* make module work

* some more organizational stuff

* some more changes

* it compiles

* using free_surface_displacement_field

* include g_Earth

* now it should run

* a little bit of cleanup

* tests should run now

* Update Project.toml

* conceptually this is better

* fix checkpointer test

* fix split explicit settings

* fixing some more tests

* import with_halo

* back on Enzyme

* fix tets

* some fixes

* these are prognostic fields

* comment

* new commit

* better

* update

* fix tests

* move functions to correct file

* apply regionally

* fix tests + unify implementation

* remove old code

* fix comment

* bugfix

* correction

* another bugfix

* switch right and left connected

* all inside apply regionally

* revert

* tests corrected

* fixed tests

* explicit free surface tendency in its own file

* test checkpointer

* last bugfix

* removed test script

* last bugfix?

* ok let's go

* add to docs

* start changing everything

* continue

* bugfix

* continue

* getting there...

* proceding

* thought I had already fixed this

* bugfix

* unpacking all the fields

* last bugfix

* fix typo in docs

* remove stray spaces

* empty line

* minor

* add reference

* split lines for math rendering

* better latex rendering

* math rendering

* Update docs/src/appendix/library.md

Co-authored-by: Gregory L. Wagner <[email protected]>

* better comment for the corrector

* add comments in the docstring

* remove kernel_parameters from the initial constructor

* Update src/Models/HydrostaticFreeSurfaceModels/SplitExplicitFreeSurfaces/split_explicit_free_surface.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* move errors inside constructors

* change summary strings

* store instead of advance

* change name to setup_free_surface!

* simplify ab2_step_G

* move `compute_free_surface_tendency!` where it should go

* minimize using statements

* bugfix

* cleaner

* should work nicely

* bugfix in integrated tendencies

* rk3 working

* think about it a bit more

* try putting it back in the ab2step

* revert quickly to see if this works

* compute_free_surface_tendecny! is the only problem

* no need for const c and const f

* merge

* adapted to explicit

* some improvements

* fix errors

* fix rk3

* bugfix

* eliding NaNs + separate compute_slow_tendencies

* whoops wrong name

* correct compute tendencies

* should work?

* ok this works!

* works

* works nicely

* improvements

* warnings

* no need for this extra defnition

* no more `implicit_free_surface_step!`

* fix bugs

* fix test

* CFL changes

* ready PR for merging

* remove duplicate code

* fix AB2

* Update split_hydrostatic_runge_kutta_3.jl

* Update src/TimeSteppers/split_hydrostatic_runge_kutta_3.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* S⁻ -> Ψ⁻

* small bugfix

---------

Co-authored-by: Gregory L. Wagner <[email protected]>
Co-authored-by: Navid C. Constantinou <[email protected]>
* fix positivity preserving WENO

* add a test

* Update src/Advection/weno_reconstruction.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* fix tests

* fix tests

---------

Co-authored-by: Gregory L. Wagner <[email protected]>
* correct windowed_time_average.jl

* test the function test_netcdf_time_averaging in test_netcdf_output_writer.jl

* test checkpointer

* change T from sch.interval to sch.window

* add progress messages and test cheeckpointer)

* print progress message every iteration

* add previous_interval_stop_time back & redefine actuation time

* update advance_time_average! and schedule

* manually force actuations to be the right value

* update test_netcdf_timeaverage.jl

* Remove `test_netcdf_timeaverage.jl`

* Change timestep in time-averaging-interval test to flag rounding errors

@liuchihl found that this test was erroneously passing because the timestep that
was chosen just happened to not be very prone to the rounding errors
that cause this bug: CliMA#3670

I have changed the timestep to the nearby value of 0.01, which should be
more useful in testing against bugs related floating point rounding errors
in the future!

* get rid of trailing white spaces

* Add warning about stride>1 and add test for JLD2 output writer

Since the reformulation of `AveragedTimeInterval` in CliMA#3721
schedules the OutputWriter based on model clock time rather than
iteration numbers, the `stride` parameter (which is inherently
based on iteration number) is somewhat ambiguous. For instance,
since the model sometimes takes extremely short timesteps due to
floating point rounding errors, a "stride" of two can mean either
2 times the nominal timestep or just 1 times the nominal timestep.
This decoupling of iteration number and model clock time, which
would presumably be worse when using a timestep wizard that produces
highly variable timesteps, can introduce significant errors into
`AveragedTimeInterval` results when stride>1.

I have added a warning that users should be cautious about using
`AveragedTimeInterval` with `stride` set to anything but the default
value of 1.

The NetCDFOutputWriter test of `AveragedTimeInterval` has now also
been ported over as a test of the JLD2OutputWriter. The parameter
setting of `stride=2` has been changed to `stride=1`, motivated
by the discussion above.

* fix typo

* undo fix typo

* fix typo

* Address Greg's comments

Starting with https://github.com/CliMA/Oceananigans.jl/pull/3721/files/9eb40759b617e168dbf0920c00bc08497ac2d29d#r1845280549

---------

Co-authored-by: Tomas Chor <[email protected]>
Co-authored-by: Henri Drake <[email protected]>
* Added momentum equation test

* Update test_enzyme.jl

---------

Co-authored-by: William Moses <[email protected]>
Co-authored-by: Gregory L. Wagner <[email protected]>
* bugfix catke

* better comment

* add catke timestepping tests

* add a test

* test fix

* fix test

* do not rewrite tendencies

* tested and everything

* more testing

* remove vestigial code

* remove vestigial code 2

* correct comment

* Bump

* rerun tests

---------

Co-authored-by: Gregory Wagner <[email protected]>
…#3955)

* small correction

* correction

* add a test

* weird

* introduce an error for dz

* bugfix

* improve grid metrics

* remove ψspacing...

* remove `AbstractMetric`

* better formatting

* continue with the spacings

* a bit of cleanup in the operators

* comment

* fixes

* last one

* simplification

* coorect import

* correct areas

* better naming

* correct grid metric

* space

* bugfix

* correct 3D vertical areas

* another bugfix

* remove all spacing functions

* tests should pass now

* correct bottom height

* fix tests

* fix example

* export the verbose versions

* revert partial cell bottom

* bugfix

* fix tests

* Update spacings_and_areas_and_volumes.jl

* fix docstrings

* better organization

* remove comment

* correct docstring

* Update Operators.jl

* correction

* correct this later

* fix docs

* correcting also immersed spacings

* import correct metrics

* import spacings

* add comment

* doctests

---------

Co-authored-by: Navid C. Constantinou <[email protected]>
* polar boundary conditions

* do not touch face fields

* correct tests
…liMA#3867)

* Add test for hydrostatic turbulence

* Bypass fill_halo_regions for tuples

* Cleanup to interface

* Make the test better

* Updates

* Fix z-coord bug

* Few more bugs

* Fix bug in field_tuples

* Tiny cleanup
;

* Another bug

* Back to pure ab2

* Make set! more differentiable

* Restore syntax for some reason

* More appropriate test w tolerance

* hopefully fix type stability issue with set!(u, ::Number)

* Updates

* Update Enzyme

* Down Enzyme

* Changed FD computation to central difference instead of forward difference. Central difference FD appears consistent with autodiff

* Switched named of variables for FD comparison

* Set AD test to be at ν1 = ν₀ + Δν to avoid a global minimum

* Removed extraneous @show statements

* Testing to see if removing grid from the handle of tupled_fill_halo_regions! is the issue

* Another test with grid handles

* Update src/Fields/field_tuples.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* More robust test for grid

* Modularizing different tupled_fill_halo_regions! methods

* Update src/Fields/field_tuples.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* Fixed variable name conflict error

* Re-running CIT tests since they didn't complete

* Update set_hydrostatic_free_surface_model.jl

---------

Co-authored-by: Joseph Kump <[email protected]>
* this should work

* remove all the @show

* restart tests

* will this work?

* test more than one position

* bugfix

* this should work

* remove unused truncate

* new interpolate

* I think this works

* try option 2

* no need for flip anymore

* fix gpu tests

* change comment and restore docstring

* fix the interpolate

* add (nothing nothing nothing) interpolate

* do not reuse names

* slogging along

* implementation is super simple

* works very well

* works, nice

* starting the change

* make it work

* works

* works but it's slow

* works nicely

* works nicely

* formatting

* including one adapt

* change to κ and test difference

* bugfix

* using ZeroField

* import correct stuff

* add docstring

* flat dimension

* add validarion case

* does this work?

* assumption

* formatting

* incorporate in ISSD

* correct tests

* only one skew diffusivity

* bugfix

* better comment

* change docstring

* more fixes

* fix testing

* keep same config for diffusive formulation

* error if we input a namedtuple

* bugfix

* extend for prescribed velocities

* correct immersed baroclinic adjustment

* implement suggestions
The unit test tests that (1) the functionality works (2) background flux divergence makes the result different
@glwagner
Copy link
Member

glwagner commented Jan 4, 2025

Why does this PR change so many files? Do either the target branch or this source branch need updating?

@liuchihl
Copy link
Contributor Author

liuchihl commented Jan 5, 2025

Yeah, I only meant to add the unit test. Since you have already updated the target branch, I will update the source branch.

@liuchihl liuchihl closed this Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants