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.

Sign up for GitHub

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

Jump to bottom

Bug Flag cleanup #1089

Merged
merged 7 commits into from
Dec 23, 2022
Merged

Bug Flag cleanup #1089

merged 7 commits into from
Dec 23, 2022

Conversation

laurenchilutti
Copy link
Contributor

@laurenchilutti laurenchilutti commented Dec 9, 2022 •
edited
Loading

Description
The following are outdated flags used in FMS that I am phasing out:

  • grid_center_bug
  • read_data_bug
  • retain_cm3_bug
  • reproduce_siena

NOT REMOVING:
reproduce_null_char_bug_flag: needed by MOM6 until MOM6 switches to fms2io
make_exchange_reproduce: a feature of FMS that needs to remain

In this PR I have removed any code associated with these flags, but am allowing the flags to still be defined in a namelist as long as the flag is set to .false. . If set to .true., a fatal will be issued.

Fixes #1093

How Has This Been Tested?
Tested via make check on Gaea
Tested with land model intel19 on gaea

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes

@laurenchilutti laurenchilutti marked this pull request as draft December 9, 2022 20:26
@mlee03
Copy link
Contributor

mlee03 commented Dec 9, 2022

@laurenchilutti make_exchange_reproduce might need to stay too

bensonr
bensonr reviewed Dec 12, 2022
View reviewed changes
Copy link
Contributor

@bensonr bensonr left a comment

Choose a reason for hiding this comment

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

All is good except the changes to make_exchange_reproduce. That is a feature used for perfornance and not a bug or workaround. Please restore this functionality.

@thomas-robinson
Copy link
Member

You need to create an issue for this PR and link it in the description.

uramirez8707
uramirez8707 requested changes Dec 19, 2022
View reviewed changes
@mlee03
Copy link
Contributor

mlee03 commented Dec 19, 2022

@laurenchilutti, all the pe's are writing out the error messages.

@laurenchilutti
Copy link
Contributor Author

@thomas-robinson @rem1776 Could one of you add MiKyung as a reviewer on this? I don;t have approproate permissions in this repository to add reviewers.

And for everyone reviewing: I have changes I'm ready to push as soon as my land model test finishes.

@rem1776 rem1776 requested a review from mlee03 December 19, 2022 18:25
mlee03
mlee03 reviewed Dec 19, 2022
View reviewed changes
@rem1776 rem1776 linked an issue Dec 19, 2022 that may be closed by this pull request
Remove retain_cm3_bug in interpolator #1004
Closed
@laurenchilutti laurenchilutti marked this pull request as ready for review December 19, 2022 18:27
@laurenchilutti
retain a default value during initialization of bug flags, and reorga…
0b6cf02 
…nize nested if loop for mpp_error calls related to deprecated bug flags
@laurenchilutti
Copy link
Contributor Author

@mlee03 @uramirez8707 My latest commit should address all comments so far. Let me know if you see any other improvements I could make to the code

thomas-robinson
thomas-robinson requested changes Dec 20, 2022
View reviewed changes
Copy link
Member

@thomas-robinson thomas-robinson left a comment

Choose a reason for hiding this comment

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

The mpp_error fatal should be called by all of the cores. This should be changed.

When you change the indents, you take responsibility for those lines of code. This doesn't need to be changed, but you should be aware.

data_override/data_override.F90 Outdated
@@ -201,6 +201,13 @@ subroutine data_override_init(Atm_domain_in, Ocean_domain_in, Ice_domain_in, Lan
unit = stdlog()
write(unit, data_override_nml)

! grid_center_bug is no longer supported.
if (grid_center_bug) then
if (mpp_pe() == mpp_root_pe()) call mpp_error(FATAL, "data_override_init: You have overridden the default " // &
Copy link
Member

Choose a reason for hiding this comment

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

why is only mpp_root_pe calling the fatal?

horiz_interp/horiz_interp.F90 Outdated
"and set it to .true. in horiz_interp_nml. This is a temporary workaround to " // &
"allow for consistency in continuing experiments. Please remove this namelist " )
if (reproduce_siena) then
if (mpp_pe() == mpp_root_pe()) call mpp_error(FATAL, "horiz_interp_mod: You have overridden the default " // &
Copy link
Member

Choose a reason for hiding this comment

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

why is only mpp_root_pe calling the fatal?

Copy link
Contributor

Choose a reason for hiding this comment

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

@thomas-robinson, that's my fault. The error messages were being written out by all the pe's

Copy link
Member

Choose a reason for hiding this comment

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

@mlee03 Yes printing the error is good for a FATAL message. All of the PEs should hit this, call MPI_ABORT in mpp_error, and produce the same tracebacks. If some of them go past this, it will create different tracebacks for different cores even though they all experienced the same issue.

The only time you want to potentially limit a call to mpp_error would be if you are calling a WARNING, especially in a loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this, now that I have a better understanding I will go ahead and remove the if (mpp_pe() == mpp_root_pe()) preceding all of the FATAL errors.

interpolator/interpolator.F90 Outdated
Comment on lines 458 to 462
call write_version_number("INTERPOLATOR_MOD", version)

if (mpp_pe() == mpp_root_pe() ) write (stdlog(), nml=interpolator_nml)
if (mpp_pe() == mpp_root_pe() ) write (stdlog(), nml=interpolator_nml)

module_is_initialized = .true.
module_is_initialized = .true.
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, BUT now you own these lines of code. git blame will show up as you.

@laurenchilutti laurenchilutti requested review from thomas-robinson and uramirez8707 and removed request for thomas-robinson and uramirez8707 December 20, 2022 17:41
@laurenchilutti laurenchilutti requested review from thomas-robinson and mlee03 and removed request for thomas-robinson December 20, 2022 17:42
@rem1776 rem1776 merged commit c3c56c9 into NOAA-GFDL:main Dec 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bensonr bensonr bensonr left review comments

@thomas-robinson thomas-robinson thomas-robinson approved these changes

@rem1776 rem1776 rem1776 approved these changes

@uramirez8707 uramirez8707 uramirez8707 approved these changes

@mlee03 mlee03 mlee03 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove bug variables Remove retain_cm3_bug in interpolator
6 participants
@laurenchilutti @mlee03 @thomas-robinson @bensonr @rem1776 @uramirez8707