New and more correct turbulence closures #383
Conversation
…ground diffusivities to 0
…roduces more accurate approximation for near-boundary diffusivity in amd
Merge branch 'master' into new-closures
…C, 35psu water, comments, references
glwagner
added
science 🌊
Codecov Report
@@ Coverage Diff @@
## master #383 +/- ##
==========================================
- Coverage 78.37% 70.15% -8.22%
==========================================
Files 22 23 +1
Lines 1244 1528 +284
==========================================
+ Hits 975 1072 +97
- Misses 269 456 +187
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #383 +/- ##
=========================================
- Coverage 78.37% 70.2% -8.18%
=========================================
Files 22 23 +1
Lines 1244 1527 +283
=========================================
+ Hits 975 1072 +97
- Misses 269 455 +186
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Nice! Just left some minor comments but otherwise looks good to merge. Smag test isn't skipped anymore so we should be back to 0 broken tests now.
We should release v0.10.0 once this is merged.
Just some comments:
Do we have an idea of what the Poincaré constant C should be in AMD? Or maybe it's something that needs to be investigated depending on the numerical methods used? Does the advection scheme and temporal discretization matter or is it mainly the numerical method (e.g. finite volume vs. spectral)?
In particular, I notice that Rozema AMD uses C=0.33 and Versteppen AMD uses C=1/12 which are pretty different.
Presumably the same question applies to Smag, although @jm-c would know what constant to use for constant Smag combined with our numerical methods as it has been used in the MITgcm.
General concerns:
- We've already discussed whether we want to maintain multiple versions of AMD And Smag but maybe what to keep will become clear with time. Maybe they have different use applications so have multiple versions could be a nice feature? Presumably a lot of code is shared between the two AMD closures which, if merged, would make maintaining both easy.
- Still not sure why
TurbulenceClosuresuses it's own differentiation and interpolation operators when we haveOceananigans.Operators. They'll have to be rewritten in finite volume form when we move to a stretched grid (PRs Finite volume operators through multiple dispatch #283 ) so maybe that'll be an opportunity to unify both sets of operators.
| # at 20ᵒC and 35 psu, according to Sharqawy et al., "Thermophysical | ||
| # properties of seawater: A review of existing correlations and data" (2010). | ||
| const ν₀ = 1.05e-6 | ||
| const κ₀ = 1.46e-7 |
There was a problem hiding this comment.
Would a subscript m be better to denote that these are molecular values?
const νₘ = 1.05e-6
const κₘ = 1.46e-7There was a problem hiding this comment.
We can do that --- I would keep the 0, however, to emphasize that these values are chosen for a particular 'state', e.g. a temperature of 20C and a salinity of 35 psu. If you agree with this logic I will make these changes.
There was a problem hiding this comment.
Ah ok if 0 refers to the reference state then that makes sense. I might include a comment explaining this but not really necessary.
There was a problem hiding this comment.
@ali-ramadhan I have included the comment
# Approximate viscosities and thermal diffusivities for seawater
# at 20ᵒC and 35 psu, according to Sharqawy et al., "Thermophysical
# properties of seawater: A review of existing correlations and data" (2010).
What do you think?
There was a problem hiding this comment.
Yeah I saw that. I guess the 0 subscript was bothering me because it could imply many things. I might be more explicit about 0 implying reference state, but this is a minor comment. How many people are going to read through that file?
| ##### | ||
|
|
||
| ##### | ||
| ##### the heinous |
|
@ali-ramadhan the constants have slightly different meaning, which creates confusion --- let me explain:
where r and q are defined in equations (9) and (11) in Rozema et al. (2015)
Thus in this case On anisotropic grids, things can be slightly different. I'm open to changing the implementation, but I'm not sure what the best route is. Possibly we will phase out the Rozema version... ? |
We were told that Smagorinsky has not been tested in MITgcm so I'm not sure that's true.
Migrating to the other operators would have taken some time and effort and possibly involved bugs so I chose to keep the ones I was familiar with. I find the new operators a little more difficult to read than the ones we currently use, and I didn't want to have to worry about that when implementing complex new functions involved with the |
Fair enough, I realize this was a decision made back when AMD was first implemented. Hopefully we'll find a way to merge the two in the future. |
|
Thanks for explaining why the Not sure where it comes from but shouldn't it be
Not sure how much code overlaps between the two, but if both are deemed important enough to maintain, then the shared functionality can go into |
Typo. 4 * 1/12 = 4/12 = 0.33. |
|
Looks good to merge! |
I considered this when I started developing the Verstappen closure. Almost none of the code is shared, which is why there are two files, unlike the Smagorinsky closures which have a lot of shared code. In principle it might be possible to refactor both implementations to share (a bit) more code but that would be a lot of work, and would also probably make both implementations more complex... |
Didn't realize they were that different. In that case, maybe makes more sense to keep two separate files. More complex implementation of AMD sounds like a bad idea. |
The difference between them is that both derivatives and velocity fields are normalized by Verstappen, while only derivatives are normalized by Rozema. To implement the Verstappen functions, I redefined functions that compute velocity gradients and tracer gradients by prefixing them with |
|
@ali-ramadhan probably a good idea to have a quick glance through to see what I am talking about. |
|
@ali-ramadhan feel free to hit the red button when you are ready. |
New and more correct turbulence closures Former-commit-id: 3f5739e
This PR makes a few major contributions:
AnisotropicMinimumDissipationAnisotropicMinimumDissipationis now calledRozemaAnisotropicMinimumDissipation(it was first described by Rozema et al. (2015)). It is not exported, but can be accessed viaOceananigans.RozemaAnisotropicMinimumDissipation.BlasiusSmagorinskywhich is untested and unexported. It can be accessed viaOceananigans.BlasiusSmagorinsky.