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

Better variables for updated masks in fill_miss_2d #602

Merged

Conversation

Hallberg-NOAA
Copy link
Member

Within fill_miss_2d, there are 2 sets of masked values used for remapping - masked values in neighboring points in arbitrary units (with names like "south") and the updated valid mask with values of 0 or 1 (with names like "gs"). At one point in the code, the former were being used where the latter were more appropriate. This commit changes this to help with the understandability of the code. All answers and output are bitwise identical.

@Hallberg-NOAA Hallberg-NOAA added documentation Improvements or additions to documentation refactor Code cleanup with no changes in functionality or results labels Apr 17, 2024
Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

I agree that this follows the intended use of the variables.

Copy link
Member

@adcroft adcroft left a comment

Choose a reason for hiding this comment

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

PGI (nVidia) has a regression in OM4_025 and other experiments. See https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/jobs/126178

@Hallberg-NOAA
Copy link
Member Author

I don't see how to revise this proposal to avoid these changes in answers with the pgi (nVidia) compiler. After looking into this in detail, the answer changes only occur in cases that use an older formulation that does a 5-point sum without any parentheses. This give the compiler the ability to rearrange this sum at will, and the nVidia compiler appears to be doing so based on considerations that are not apparent to me. However, when the newer (rotationally symmetric) but mathematically equivalent form with parentheses that specify the order of operations are used, the answers are preserved for all of the compilers.

  Within fill_miss_2d, there are 2 sets of masked values used for remapping -
masked values in neighboring points in arbitrary units (with names like "south")
and the updated valid mask with values of 0 or 1 (with names like "gs").  At one
point in the code, the former were being used where the latter were more
appropriate.  This commit changes this to help with the understandability of the
code.

  However, when the order of arithmetic is not specified with parentheses, the
Nvidia compiler is changing answers with this change of variables.  To avoid
this change in answers, this commit is only changing which variables are used
when the answer_date is larger than 20190101 and the order of the sums are
specified with parentheses.  This commit also now moves the logical test for
ans_2018 outside of the i- and j- loops where non-zero values of a_chg are set.

  All answers and output are bitwise identical.
@Hallberg-NOAA Hallberg-NOAA force-pushed the hor_regrid_fill_goodval branch from a2b5dfd to 8c161ec Compare May 5, 2024 23:40
@Hallberg-NOAA
Copy link
Member Author

I have revised this commit to avoid the Nvidia changes by only changing which variables we are using when the relevant answer_date argument is high enough that the order of arithmetic is specified. I also slightly refactored this code at the same time to move the logical test on ans_2018 is outside of the i- and j- do loops.

Copy link
Member

@marshallward marshallward left a comment

Choose a reason for hiding this comment

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

While it would be nice to better understand why a variable swap would change the order of arithmetic (if that is even what is happening here), I suppose it does not much matter in the end, and this will preserve the solutions.

@marshallward
Copy link
Member

Gaea regression: https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/23564 ✔️

@marshallward marshallward merged commit 1cf20b3 into NOAA-GFDL:dev/gfdl May 22, 2024
10 checks passed
@Hallberg-NOAA Hallberg-NOAA deleted the hor_regrid_fill_goodval branch May 24, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation refactor Code cleanup with no changes in functionality or results
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants