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

Feature/pnorris/#422 #443 rrtmg lw sw combo generator #492

Merged

Conversation

dr0cloud
Copy link
Contributor

This is a large non-zero-diff PR with fastLW+SW that tidies up RRTMG LW and SW and makes them both about 2/3 the runtime and makes them use a combined cloud generator. The changes have been discussed and vetted in modeling group over the past 2-3 months. Most changes are "statistically zero-diff" in the sense that they have minimal difference (~< 0.01 W/m2) in global TOA fluxes (in 3 month MERRA-2 replays) and minimal changes in zonal means. But any difference in the stochastic cloud generator (even changing the PRNG) will have large instantaneous changes in clouds (and therefore radiative fluxes) that will average to near-zero over space and time. The few changes that were statistically non-zero-diff have been understood as being better.

dr0cloud added 30 commits May 9, 2021 15:54
@dr0cloud dr0cloud added the Non 0-diff The changes in this pull request are non-zero-diff label Nov 29, 2021
@dr0cloud dr0cloud requested review from mathomp4 and tclune November 29, 2021 16:32
@dr0cloud dr0cloud requested review from a team as code owners November 29, 2021 16:32
@mathomp4 mathomp4 requested a review from wmputman November 29, 2021 16:36
@dr0cloud
Copy link
Contributor Author

Scott, please also merge to ORBIT default on (AGCM.rc add two lines) at or near this PR (before or after). Thanks, Peter.

tclune
tclune previously approved these changes Nov 29, 2021
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.

CMake changes

@mathomp4
Copy link
Member

mathomp4 commented Nov 29, 2021

@dr0cloud It looks like the GNU build has an issue:

/root/project/GEOSgcm/src/Components/@GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSradiation_GridComp/GEOS_RadiationShared/cloud_condensate_inhomogeneity.F90:95:15:

   95 |       if (.not. condensate_inhomogeneous()) then
      |               1
Error: Function 'condensate_inhomogeneous' at (1) has no IMPLICIT type

Now the function in question does seem pretty...correct:

   function condensate_inhomogeneous result (inhomo)
      logical :: inhomo
      if (.not. initialized) then
         write(error_unit,*) 'file:', __FILE__, ', line:', __LINE__
         error stop 'must initialize_inhomogeneity first'
      end if 
      inhomo = (inhm > 0)
   end function condensate_inhomogeneous

so I'm not sure why GNU is having an issue.

@tclune do you see anything wrong here? Or is this just some GNU oddity so it needs to be the uglier:

   logical function condensate_inhomogeneous()
      if (.not. initialized) then
         write(error_unit,*) 'file:', __FILE__, ', line:', __LINE__
         error stop 'must initialize_inhomogeneity first'
      end if 
      condensate_inhomogeneous = (inhm > 0)
   end function condensate_inhomogeneous

@mathomp4
Copy link
Member

Per @Gvilla1000-nasa, he says that this might be a GNU bug. He encountered something similar with an:

  if (function()) then

where he instead had to do:

  condition = function()
  if (condition) then

GNU had some issue with doing an if-test on a function itself. (But apparently Intel and NAG were okay with doing that.)

@mathomp4
Copy link
Member

Scott, please also merge to ORBIT default on (AGCM.rc add two lines) at or near this PR (before or after). Thanks, Peter.

@sdrabenh Just making sure you see this. @dr0cloud can you put the two lines here? Or do you have a PR in GEOSgcm_App that does it?

@sdrabenh
Copy link
Collaborator

Scott, please also merge to ORBIT default on (AGCM.rc add two lines) at or near this PR (before or after). Thanks, Peter.

@sdrabenh Just making sure you see this. @dr0cloud can you put the two lines here? Or do you have a PR in GEOSgcm_App that does it?

@mathomp4 and @dr0cloud I can add these changes to this PR

@dr0cloud
Copy link
Contributor Author

dr0cloud commented Nov 29, 2021

@sdrabenh @mathomp4 Thanks Scott. And Matt, do we need to do anything about this GCC bug? Or can we just leave it as is?

@mathomp4
Copy link
Member

@sdrabenh @mathomp4 Thanks Scott. And Matt, do we need to do anything about this GCC bug? Or can we just leave it as is?

@dr0cloud I'm doing some testing now to try and figure out the "right fix" to suggest you try out.

@mathomp4
Copy link
Member

@dr0cloud Well, the solution was pretty simple. I think we just need to make:

   function condensate_inhomogeneous result (inhomo)

be:

   function condensate_inhomogeneous() result (inhomo)

For some reason, parallel make is "hiding" the real error which can be seen in serial build. When I make that change, it seems to build. I also then did a test run of GEOS (GNU Debug) and, well, nothing crashed. So that's nice.

@dr0cloud
Copy link
Contributor Author

@mathomp4 Matt fixed it .. need "function f() result (r)" ... not "function f result (r)" for GCC. Thanks Matt!

@mathomp4 mathomp4 requested a review from a team December 17, 2021 20:15
@sdrabenh sdrabenh merged commit 7961301 into develop Dec 17, 2021
@mathomp4 mathomp4 deleted the feature/pnorris/#422-#443-RRTMG-LW-SW-combo-generator branch December 21, 2021 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Non 0-diff The changes in this pull request are non-zero-diff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants