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

Add check for nans config option #386

Merged
merged 15 commits into from
May 24, 2023

Conversation

jedwards4b
Copy link
Collaborator

@jedwards4b jedwards4b commented May 19, 2023

Description of changes

Add check_for_nans to the nuopc.runconfig

Specific notes

Contributors other than yourself, if any:

CMEPS Issues Fixed: #384

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

Any User Interface Changes (namelist or namelist defaults changes)? Add
logical variable check_for_nans modifiable in user_nl_cpl.

Testing performed

Please describe the tests along with the target model and machine(s)
If possible, please also added hashes that were used in the testing
Tested using cesm2_3_alpha13b
PFS.ne30pg3_t061.B1850MOM.cheyenne_intel.allactive-defaultio

@jedwards4b jedwards4b self-assigned this May 19, 2023
@jedwards4b
Copy link
Collaborator Author

I've introduced a circular dependency - will need to address.

mediator/med_methods_mod.F90 Outdated Show resolved Hide resolved
mediator/med_phases_prep_atm_mod.F90 Outdated Show resolved Hide resolved
@billsacks
Copy link
Member

Thanks for adding this flag @jedwards4b ! Given the performance impact documented in #377 (comment) (4.5% slowdown for a typical production case), I would vote for making the default be to not check for NaNs - or maybe to make the default be to check for NaNs in debug cases but not in production cases. My thinking is that, in most cases, NaNs will be detected eventually, so this CMEPS check is more about detecting them as soon as possible... so people can always rerun with the check enabled in these situations.

Comment on lines 2533 to 2538
#ifndef CESMCOUPLED
! For now only CESM uses shr_infnan_isnan - so until other models provide this
RETURN
mediator_checkfornans = .false.
#endif

if(.not. mediator_checkfornans) return

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the change in how you set the config variable, we can remove the CESMCOUPLED ifdef here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I haven't done that yet is that the github ext test doesn't include the cdeps library and thus the share code. I am working on updating the github test to include that and then will remove the ifdefs once I have that working. I got pulled into another issue though so it may be a day or two before I can get back to this, for now I'll convert this PR to draft.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, thanks. I was worried I was holding you up by not testing this yet.

@jedwards4b jedwards4b marked this pull request as draft May 23, 2023 16:58
@jedwards4b jedwards4b marked this pull request as ready for review May 24, 2023 16:07
@jedwards4b
Copy link
Collaborator Author

I have added the cdeps build to the github workflow and removed the CESMCOUPLED ifdefs from med_methods_mod.F90. I think this is ready to merge.

Copy link
Collaborator

@DeniseWorthen DeniseWorthen left a comment

Choose a reason for hiding this comment

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

This is working for UFS with the flag to check either on or off. Thanks.

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.

Looks good. Thanks for the new implementation.

@jedwards4b jedwards4b merged commit 06b79ff into ESCOMP:main May 24, 2023
@jedwards4b jedwards4b deleted the add_check_for_nans_config_option branch May 24, 2023 19:04
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 a namelist variable to control check_for_nans feature
4 participants