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

Improves DynamicCoefficient docstring with discussion about schedule #4075

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,54 @@ in the `x` and `y` directions).

`DynamicCoefficient` is updated according to `schedule`, and `minimum_numerator` defines the minimum
value that is acceptable in the denominator of the final calculation.

Examples
========

```jldoctest
julia> using Oceananigans

julia> dynamic_coeff = DynamicCoefficient(averaging=(1, 2))
DynamicCoefficient with
├── averaging = (1, 2)
├── schedule = IterationInterval(1, 0)
└── minimum_numerator = 1.0e-32

julia> dynamic_smagorinsky = Smagorinsky(coefficient=dynamic_coeff)
Smagorinsky closure with
├── coefficient = DynamicCoefficient(averaging = (1, 2), schedule = IterationInterval(1, 0))
└── Pr = 1.0
```

The dynamic Smagorinsky above has its dynamic coefficient recalculated at every time
step. While this provides the highest level of accuracy, it will almost certainly be very slow
given the high cost of calculating `DynamicCoefficient`s. Because of this slowdown, it is common in the
literature to recalculate the coefficient only every few time steps, with the assumption that the
their values don't change much from one time-step to the other. Usually the update frequency chosen
Copy link
Member

Choose a reason for hiding this comment

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

If we say "usually" then we need citation. Eg wikipedia might say [citation needed]. I think a better practice is to give a range so we don't multiply the "collective intellectual debt" that always choosing the same number is having on this community.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say try strides of 2-8 or something.

Copy link
Collaborator Author

@tomchor tomchor Feb 3, 2025

Choose a reason for hiding this comment

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

I can definitely add some citations. The thing is that I have never seen any other update frequency used in any paper. If you have info on papers that do it differently, let me know and we can give a range. Otherwise, the only number that has been used (afaik) is 5.

And again, I understand the rationale behind going by factors of 2, but it misses the one number that has been widely tested haha.

So please send some papers with different update frequencies my way if you know any, but otherwise I'd address your comment by keeping the text the same and adding references so that users can follow up and make their own conclusions. How does that sound?

is every 5 steps, which considerably speeds up simulations and has been empirically shown to produce
very similar results to having it be updated at every time step. We can achieve this by using the
`schedule` keyword argument such as:

```jldoctest
julia> using Oceananigans

julia> dynamic_coeff = DynamicCoefficient(averaging=(1, 2), schedule=IterationInterval(5))
DynamicCoefficient with
├── averaging = (1, 2)
├── schedule = IterationInterval(5, 0)
└── minimum_numerator = 1.0e-32

julia> dynamic_smagorinsky = Smagorinsky(coefficient=dynamic_coeff)
Smagorinsky closure with
├── coefficient = DynamicCoefficient(averaging = (1, 2), schedule = IterationInterval(5, 0))
└── Pr = 1.0
```

References
==========

Bou-Zeid, Elie, Meneveau, Charles, and Parlange, Marc. (2005) A scale-dependent Lagrangian dynamic model for
large eddy simulation of complex turbulent flows, **Physics of Fluids_, **17**, 025105.
"""
function DynamicCoefficient(FT=Float64; averaging, schedule=IterationInterval(1), minimum_numerator=1e-32)
minimum_numerator = convert(FT, minimum_numerator)
Expand Down