-
Notifications
You must be signed in to change notification settings - Fork 7
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
[Feature] delnflux
all nord
#3
Conversation
GEOS/FV3GFS: d2_highord_stencil ops order difference
Add extra check on the value of nord which is taken as an hypothesis downstream
delnflux
all nord
Vorticity damping GEOS differs from FV3GFS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question for Lucas, one question for Florian, and one change to d_sw
): | ||
with computation(PARALLEL), interval(...): | ||
if nord > current_nord: | ||
d2 = ((fx - fx[1, 0, 0]) + (fy - fy[0, 1, 0])) * rarea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lharris4 Mathematically this is the same as the version used in SHiELD, but with parens around the fx/fy subtraction to enforce the order of operations. Is it ok to merge these together or should we keep separate versions for GEOS and NOAA applications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is OK although the former may make it easier for the compiler to optimize. Is there any particular reason to do it this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At double floating precision, this has minimal to no impact. At single precision, however the order of the subtraction has measurable impacts (especially because of close-to-zero behavior of a lot of those codes).
A large amount of those diff pattern have been corrected in the GEOS fork of FV3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. We'd like to include a number of these in the GFDL FV3 also.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. It would be nice to be able to combine the d2_higherorder stencils if possible, but that can happen in a subsequent PR. I'll open an issue for it.
Hi, Florian. Thank you for letting me know. We hadn't seen these changes in
the GEOS fork of FV3 before; I know GMAO had been working on some
order-of-operation changes to avoid rounding-error mass loss at single
precision but hadn't heard whether they were successful. This would
potentially be useful for the GFDL main branch too.
Lucas
…On Wed, Feb 21, 2024 at 9:47 AM Florian Deconinck ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pyFV3/stencils/delnflux.py
<#3 (comment)>:
>
***@***.***
-def d2_highorder(fx: FloatField, fy: FloatField, rarea: FloatField):
- d2 = (fx - fx[1, 0, 0] + fy - fy[0, 1, 0]) * rarea
- return d2
+def d2_highorder_stencil_GEOS(
+ fx: FloatField,
+ fy: FloatField,
+ rarea: FloatFieldIJ,
+ nord: FloatFieldK,
+ d2: FloatField,
+ current_nord: int,
+):
+ with computation(PARALLEL), interval(...):
+ if nord > current_nord:
+ d2 = ((fx - fx[1, 0, 0]) + (fy - fy[0, 1, 0])) * rarea
At double floating precision, this has minimal to no impact. At single
precision, however the order of the subtraction has measurable impacts
(especially because of close-to-zero behavior of a lot of those codes).
A large amount of those diff pattern have been corrected in the GEOS fork
of FV3.
—
Reply to this email directly, view it on GitHub
<#3 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMUQRVCDHVYMVT2PM3STD4LYUYCGJAVCNFSM6AAAAABDAOETFKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJTGQYTENRTGU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
My pleasure. If this project can have the side effect of updating people on work that hasn't been diffused then I am doubly glad! I'll chat with Bill about it before we merge this. |
As expected, those changes were part of the work to keep mass conservation at single precision. GEOS is running single precision by default for a while with good results on that side. Going forward I'll push those changes in as fixes rather than code differences between the two agencies. |
@oelbert Ready for merge (once tests come back happy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
): | ||
with computation(PARALLEL), interval(...): | ||
if nord > current_nord: | ||
d2 = ((fx - fx[1, 0, 0]) + (fy - fy[0, 1, 0])) * rarea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. We'd like to include a number of these in the GFDL FV3 also.
@@ -1240,38 +639,40 @@ def __call__(self, q, fx2, fy2, damp_c, d2, mass=None): | |||
""" | |||
|
|||
if mass is None: | |||
self._d2_damp(q, d2, damp_c) | |||
self._d2_damp(q=q, d2=d2, damp=damp_c, nord=self._nord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that some of these damping coefficients are layer-dependent (ie. much stronger and lower-order damping in the sponge layer).
The original port of
DelnFlux
used the 3 first values of thenord_dictionary
to compute. The original code reads thenord
value for all K-levels and decides to execute the code or not.GEOS use case showed that the assumption in
pyFV3
is incorrect. On top of that, the time "optimized" by the strategy is minimal and by virtue of the DSL should not lie in the hands of the modelers.In this PR:
We add
version.py
to introduce aIS_GEOS
switch that can be used to differentiate between numerical choices between NOAA and NASAFull
nord
coverage for DelnFluxAdd check on column calculation taken as hypothesis for downstream calculations
FV3GFS/GEOS differences solving:
d2_high_order_stencil
as a difference of orders operationsnord
&damp
column calculation goes all the way to then_sponge
layer in GEOSNOTE: to do FV3GFS/GEOS swap we use the
constant
file inndsl
, a known refactorable entity.