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

[WIP] add nan checks to prep modules #372

Closed
wants to merge 1 commit into from

Conversation

jedwards4b
Copy link
Collaborator

@jedwards4b jedwards4b commented May 3, 2023

Description of changes

Check for Nans in med_phases_prep modules

Specific notes

Contributors other than yourself, if any:

CMEPS Issues Fixed (include github issue #):
Resolves #370

Are changes expected to change answers? (specify if bfb, different at roundoff, more substantial)

Any User Interface Changes (namelist or namelist defaults changes)?

Testing performed

Testing performed if application target is CESM:

  • (recommended) CIME_DRIVER=nuopc scripts_regression_tests.py
    • machines:
    • details (e.g. failed tests):
  • (recommended) CESM testlist_drv.xml
    • machines and compilers:
    • details (e.g. failed tests):
  • (optional) CESM prealpha test
    • machines and compilers
    • details (e.g. failed tests):
  • (other) please described in detail
    • machines and compilers
    • details (e.g. failed tests):

Testing performed if application target is UFS-coupled:

  • (recommended) UFS-coupled testing
    • description:
    • details (e.g. failed tests):

Testing performed if application target is UFS-HAFS:

  • (recommended) UFS-HAFS testing
    • description:
    • details (e.g. failed tests):

Hashes used for testing:

  • CESM:
  • UFS-coupled, then umbrella repostiory to check out and associated hash:
    • repository to check out:
    • branch/hash:
  • UFS-HAFS, then umbrella repostiory to check out and associated hash:
    • repository to check out:
    • branch/hash:

@jedwards4b jedwards4b requested a review from mvertens May 3, 2023 13:58
@jedwards4b jedwards4b self-assigned this May 3, 2023
@jedwards4b jedwards4b marked this pull request as draft May 3, 2023 13:59
@DeniseWorthen
Copy link
Collaborator

Would this additional check be expected to have much performance impact?

@jedwards4b
Copy link
Collaborator Author

Thanks for asking @DeniseWorthen : We will confirm the performance impact and if it's significant we will either only turn on in debug mode or provide a variable for the user to toggle on or off.

@klindsay28
Copy link

It doesn't look like you're setting rc when no NaNs are found.

Also, I think it would be helpful in med_phases_prep_atm to check all fields for NaNs and then returning if NaN is found in any field, instead of returning after the first detection of NaN. That way, when more than one field has NaNs, you get a message for all of them. I think can be helpful when debugging such an occurrence.

@jedwards4b
Copy link
Collaborator Author

@klindsay28 This is an incomplete example, thanks for the message about rc.
Check and report all fields with NaN's instead of just the first one encountered. Sure we can do that, will need to refactor so that the number of NaN's found is returned to the caller and the caller is responsible for generating the error message.

Copy link
Collaborator

@mvertens mvertens left a comment

Choose a reason for hiding this comment

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

This looks good. Implementing the addition that Keith suggested would be good. However, we also want an interface where we pass in a field bundle - and it goes through each field, determines if the field has an undistributed dimension and then extracts the 1d or 2d pointer out and then calls this check routine.

@billsacks
Copy link
Member

Thank you very much for your work on this feature, @jedwards4b ! I have edited your top-level description to note that this will resolve #370.

@mvertens
Copy link
Collaborator

mvertens commented May 8, 2023

@JEdwards - can you please close this PR in favor of the PR I am working on? I did not want to unilaterally close it.

@jedwards4b
Copy link
Collaborator Author

Superseded by #377

@jedwards4b jedwards4b closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add NaN checks for coupling fields
5 participants