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

by pass missing value of NaN #1576

Merged
merged 12 commits into from
Jun 23, 2022
Merged

Conversation

weiyuan-jiang
Copy link
Contributor

@weiyuan-jiang weiyuan-jiang commented Jun 18, 2022

If the missing values is Nan, all the calculation will be Nan. It is not necessary to swap or test

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Trivial change (affects only documentation or cleanup)

Checklist:

  • I have tested this change with a run of GEOSgcm (if non-trivial)
  • I have added one of the required labels (0 diff, 0 diff trivial, 0 diff structural, non 0-diff)
  • I have updated the CHANGELOG.md accordingly following the style of Keep a Changelog

@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner June 18, 2022 18:01
@weiyuan-jiang weiyuan-jiang added the 0 Diff The changes in this pull request have verified to be zero-diff with the target branch. label Jun 18, 2022
@weiyuan-jiang
Copy link
Contributor Author

This PR is addressing the issue ufs-community/ufs-weather-model#1259

@mathomp4
Copy link
Member

I can say this is zero-diff for a C24 run of GEOSgcm. I'll let @tclune or someone more knowledgable than I comment on the code change.

@tclune
Copy link
Collaborator

tclune commented Jun 21, 2022

I don't see how this change has anything to do with NaN. The change merely verifies that if a fill is specified then it is of a numeric type. (I.e., it will ignore fills that are strings or bools.) What am I missing?

@weiyuan-jiang
Copy link
Contributor Author

@bena-nasa , I think it is fine to bypass the Nan values even there is Nan. Any calculation relults that Nan is involved would be Nan too.

@weiyuan-jiang
Copy link
Contributor Author

I don't see how this change has anything to do with NaN. The change merely verifies that if a fill is specified then it is of a numeric type. (I.e., it will ignore fills that are strings or bools.) What am I missing?

This line crashes without the fix.

call ESMF_AttributeGet(field,name=fill_value_label,value=fill_value,_RC)

I suspect this is a bug from ESMF. Just two lines above, I test the return type of the '_FillValue', it is ESMF_TYPEKIND_R4 , but ESMF crashes when trying to get it.

@weiyuan-jiang
Copy link
Contributor Author

@tclune
Copy link
Collaborator

tclune commented Jun 21, 2022

I don't see how your change would affect that case. R4 matches so has_missing_value should be .true. before and after your change. If the code change affects results, it seems to suggest memory corruption.

@weiyuan-jiang
Copy link
Contributor Author

@bena-nasa , I think it is fine to bypass the Nan values even there is Nan. Any calculation relults that Nan is involved would be Nan too.

Our Metadata does not take "Nan" as a numerical type. So there is no missing data if it is Nan. I think it is fine because the results would be Nan if Nan is involved in the calculation

@weiyuan-jiang
Copy link
Contributor Author

I have another theory. When ESMF checks the type of attribute '_FillValue', it actually checks the type of the variable. They are supposed to be the same type anyway. Whey trying to get it, it crashes because it is a string ( Nan or Nanf)

@tclune
Copy link
Collaborator

tclune commented Jun 21, 2022

Your theory is plausible, but I still don't see how the changes here have any effect.

@weiyuan-jiang
Copy link
Contributor Author

After my change, our program thinks there is no missing value if it is "Nan". So the above crashing line I showed you would not be executed.

@weiyuan-jiang
Copy link
Contributor Author

pfio can set and get Nan attribute, but ESMF does not seem to work. But I think it is safe to by pass Nan.

Copy link
Collaborator

@tclune tclune left a comment

Choose a reason for hiding this comment

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

See inline comment.

@weiyuan-jiang
Copy link
Contributor Author

If regrid gets trouble when there is Nan, this PR won't work. Nan needs to be replaced somewhere.

@weiyuan-jiang
Copy link
Contributor Author

I have verified this PR resolves the Nan issue from ufs @bena-nasa @tclune

tclune
tclune previously approved these changes Jun 22, 2022
@weiyuan-jiang weiyuan-jiang requested a review from a team as a code owner June 23, 2022 17:29
@weiyuan-jiang
Copy link
Contributor Author

@mathomp4 This CMakeLists.txt is working on Orion. If it is working here, lets merge it.

tclune
tclune previously approved these changes Jun 23, 2022
@mathomp4
Copy link
Member

Okay. Let me make an update to CMakeLists.txt with a comment.

Copy link
Member

@mathomp4 mathomp4 left a comment

Choose a reason for hiding this comment

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

@tclune has approved. This is just a "re-approval" after my CMake comment.

@mathomp4 mathomp4 merged commit 62aa921 into develop Jun 23, 2022
@mathomp4 mathomp4 deleted the feature/wjiang/by_pass_nan_missing branch June 23, 2022 20:16
@jkbk2004
Copy link

@mathomp4 are you going to release a new patched version of the mapl-2.21.3 out of this pr?

@mathomp4
Copy link
Member

@mathomp4 are you going to release a new patched version of the mapl-2.21.3 out of this pr?

@jkbk2004 I think once we get in #1580, develop should be in a good place. But let me ping the team to see if anything else needs in. If not, I'll work on getting something out today (probably MAPL 2.22).

@mathomp4 mathomp4 linked an issue Jun 24, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 Diff The changes in this pull request have verified to be zero-diff with the target branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crash when fill_vaue is not a number
5 participants