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

Horizontal regridding and stabilizing CATKE features #2881

Merged
merged 71 commits into from
Mar 17, 2023

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Jan 26, 2023

This PR adds regridding in x (and there was previously untested regridding in y). Regridding is only supported for tracer fields (though it's not a huge amount of work to extend the schemes for any field).

I believe the regridding is conservative for RectilinearGrid, and also conservative for LatitudeLongitudeGrid when the grids are aligned (ie when all grid lines of the coarser of the two grids is shared with the finer grid).

In the case of partial overlaps on curvilinear grids, the volumes of the overlaps are approximated and thus the regridding is not perfectly conservative. It could make sense to emit a warning in this case.

In principle, this should allow regridding in 3D by forming intermediate regridding in x, y, and z. We leave multi-dimensional regridding to the user for now.

This PR also adds a minimum_turbulent_kinetic_energy and negative_turbulent_kinetic_energy_damping_timescale to CATKEVerticalDiffusivity, either of which can help stabilize 2D or 3D simulations with CATKE (where due to advection TKE may go negative).

@glwagner glwagner added experimental feature 🧪 Because danger is the spice of life feature 🌟 Something new and shiny labels Jan 26, 2023
@glwagner glwagner marked this pull request as draft January 26, 2023 16:14
@navidcy
Copy link
Collaborator

navidcy commented Feb 23, 2023

the current regriding tests fail... is this alarming?

@glwagner
Copy link
Member Author

Haha, that's not good.

@navidcy
Copy link
Collaborator

navidcy commented Feb 23, 2023

merge main?

@glwagner glwagner marked this pull request as ready for review March 2, 2023 17:48
@glwagner
Copy link
Member Author

@simone-silvestri im getting a lot of errors after merging your suggestions… can you take a look?

@navidcy
Copy link
Collaborator

navidcy commented Mar 17, 2023

I'll have a look tomorrow!

vi_diffusivity_fields = Tuple(diffusivity_fields[n] for n = 1:N if is_vertically_implicit(closure[n]))

# length(vi_closure) > 1 && error("More than one closure with VerticallyImplicitTimeDiscretization is not supported")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove or is it still the case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's not tested and I suspect CATKE may not work with a tuple yet. But I think we should save for a future PR, this PR does enough already. I'll remove the comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry let me clarify -- we test tupled closures with implicit vertical diffusion and they pass (I also tested myself and found that it works). CATKE is special though, I think there is a bug with the linear component (only CATKE has this). But I'd like to debug and implement a test in another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental feature 🧪 Because danger is the spice of life feature 🌟 Something new and shiny
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants