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

Physics cap compile error with empty suite group (duplicate 'cdata') #127

Open
MicroTed opened this issue Nov 13, 2023 · 14 comments
Open

Physics cap compile error with empty suite group (duplicate 'cdata') #127

MicroTed opened this issue Nov 13, 2023 · 14 comments
Labels
bug Something isn't working

Comments

@MicroTed
Copy link
Collaborator

MicroTed commented Nov 13, 2023

Description

An empty suite group results in cdata being passed and declared twice in the init_cap function.

Steps to Reproduce

I am updating a capability to run an idealized periodic case. In order to turn off radiation, I removed the scheme calls from the radiation group (suite_FV3_mp_nssl_ideal.xml, attached as a text file):

  <group name="radiation">
    <subcycle loop="1">
    </subcycle>
  </group>

That used to work fine, but now cmake/prebuild is adding an extra 'cdata' in the call arguments [and defining it twice, the second time as intent(in)], which results in a compile error (ccpp_FV3_mp_nssl_ideal_cap.F90):

   function FV3_mp_nssl_ideal_init_cap(cdata,one,GFS_Control,levh2o,h2o_coeff,con_t0c,cdata,con_g,con_rd,con_cp, &
                  con_rv,con_cliq,con_csol,con_eps,GFS_Data) result(ierr)

      use ccpp_types, only: ccpp_t
      use GFS_typedefs, only: GFS_control_type
      use machine, only: kind_phys
      use GFS_typedefs, only: GFS_data_type

      implicit none

      integer :: ierr
      type(ccpp_t), intent(inout) :: cdata
      integer, intent(in) :: one
      type(gfs_control_type), intent(in) :: GFS_Control
      integer, intent(in) :: levh2o
      integer, intent(in) :: h2o_coeff
      real(kind_phys), intent(in) :: con_t0c
      type(ccpp_t), intent(in) :: cdata

It will compile OK if I comment the group out completely, but then I seem to get a runtime error:

An error occurred in ccpp_physics_run for group radiation, block    1 and thread    1 (ntX=   1):
Group radiation not found

Adding code to GFS_typedefs.F90 to set nsswr=-1 and nslwr=-1 works to turn off radiation as a work-around for the time being.

It also compiles and runs if I manually fix the extra cdata in the function and the call. I didn't have any problems with ccpp-framework as of commit 1b6352fb24f05 (Jan 2023), if that is any help.

Additional Context

  • Machine: Mac (intel CPU)
  • Compiler: GNU 11.3
  • Suite Definition File or Scheme: Custom

Output

Please include any relevant log files, screenshots or other output here.

suite_FV3_mp_nssl_ideal.xml.txt

@MicroTed MicroTed added the bug Something isn't working label Nov 13, 2023
@MicroTed
Copy link
Collaborator Author

@grantfirl Any thoughts on this? The prebuild problem forces the suite to have a valid radiation scheme call, so I made a work-around by adding a flag 'do_radiation' to the fv_core_nml namelist that can set the radiation interval such that radiation is never called. That works but deviates from the spirit of a suite, i.e., that a suite should contain exactly what will be used.

@grantfirl
Copy link
Collaborator

I think that this is probably my fault from NCAR/ccpp-framework#503. I'll see if I can reproduce and fix it.

@grantfirl
Copy link
Collaborator

grantfirl commented Dec 12, 2023

Although, if the group is empty, you should just be able to delete the entire group, and I think it should be OK. This could be a temp fix for you. Have you tried that?

@MicroTed
Copy link
Collaborator Author

Yes, I tried deleting the group, and that compiles, but then there is a runtime error, "Group radiation not found". Thus the need for the workaround to prevent radiation from being called.

@grantfirl
Copy link
Collaborator

Yes, I tried deleting the group, and that compiles, but then there is a runtime error, "Group radiation not found". Thus the need for the workaround to prevent radiation from being called.

Ah yes, I didn't read the description carefully enough, sorry.

@grantfirl
Copy link
Collaborator

grantfirl commented Dec 12, 2023

OK, the runtime error shows up because in the UFS, the calls for each CCPP group happen individually as opposed to the entire suite at once, as is the case with the SCM. That is, even though you remove the group from the SDF, the host model (atmos_model.F90) is still trying to call the radiation group, hence the runtime error message. You can get around this error by either editing FV3/ccpp/driver/CCPP_driver.F90 to just return when step = "radiation" or to comment out where the CCPP_step() subroutine is called with step="radiation" in FV3/atmos_model.F90.

This should solve your immediate problem, but, regardless, if there is an empty group in the SDF, the CCPP framework should be able to handle this by creating empty caps. That is perhaps an easier way for users to handle this situation than going into the host code and modifying calls.

@MicroTed
Copy link
Collaborator Author

MicroTed commented Dec 12, 2023

Maybe another tact is to just ignore the missing group error (or not generate it?). I'll look at that. It would be more satisfying and intuitive to have no radiation group at all than to have an empty one.

@MicroTed
Copy link
Collaborator Author

MicroTed commented Dec 13, 2023

Maybe a do_radiation flag is the way to go, after all. There are flags if FV3/atmos_model.F90 for calling the stochastic physics, so that the stochastics group can be absent from a suite, so why not radiation, as well? And mkstatic.py can be updated to set a specific error code (86 -- IYKYK) if a group is not found. And then I can use that error code to set the do_radiation flag if the user didn't set it in the namelist. And that way no fix is needed for the prebuild. (It could also only not be in the namelist and only set to false if that error code comes up?)

@grantfirl
Copy link
Collaborator

I think that there is a simple bugfix for making sure that cdata does not appear more than once in that argument list: NCAR/ccpp-framework#517

This should get us back to the way it worked before (still needing an empty radiation group in the SDF, but compiling and running fine). Whether or not we also need to modify the host CCPP interface to not force us to have specific groups is another question. I can bring it up with others and see what they think about your suggestions to provide more flexibility with SDFs.

@MicroTed
Copy link
Collaborator Author

Nice if it's a simple fix! Since we already have other changes to fv3 for our update to support an idealized domain, it's easy enough to include the code that detects if there is no radiation suite. (I guess I have been assuming that the SCM won't care -- correct?) That way it can work with either an empty group or no group at all.

@grantfirl
Copy link
Collaborator

Nice if it's a simple fix! Since we already have other changes to fv3 for our update to support an idealized domain, it's easy enough to include the code that detects if there is no radiation suite. (I guess I have been assuming that the SCM won't care -- correct?) That way it can work with either an empty group or no group at all.

Ya, I'd support your suggested changes to fv3 for the reasons you've mentioned. I'm going to go ahead and submit NCAR/ccpp-framework#517 as a real PR so that it can be merged in and host models can point to it, since it is a bugfix. Do you think that you really need, for example, https://github.com/NCAR/ccpp-framework/blob/0eca5c2c8885f3acadfed1f4945d19d1e97bb25f/scripts/mkstatic.py#L380C17-L380C17 changed to have a special error code? The reason why I'm reticent to do include this now is that there is momentum to switch to the next version of the CCPP framework that may have more complete error-checking and I don't want to necessarily change too much in the first version (ccpp_prebuild.py) except for bugfixes. If it is really necessary to introduce an error code different than 1 to complete the functionality in FV3 that you intend, we can bring it up with some of the other CCPP Framework folks and see if they're OK with it.

@MicroTed
Copy link
Collaborator Author

Ok, thanks for pointing that out. I suppose I don't really need to change mkstatic.py ierr if I can parse the errmsg in CCPP_driver.F90 and set that level's ierr to a unique value. I'll look into that.

@grantfirl
Copy link
Collaborator

grantfirl commented Dec 14, 2023

@MicroTed I started a discussion here: NCAR/ccpp-framework#518

If the better error handling isn't coming any time soon, perhaps we can at least add a meaningful error code besides 1 that you can use for this purpose. It makes perfect sense, I just don't want to ignore or derail other efforts that may be ongoing that could also be useful for your (and others!) purpose. We'll see if the discussion goes anywhere.

Having to put code in to parse error message strings seems pretty tedious compared to setting integer error codes.

@MicroTed
Copy link
Collaborator Author

A conditional on errmsg works just fine in the FV3 CCPP_driver.F90, so I can use that for the time being and not mess with CCPP framework.

mkavulich pushed a commit to NCAR/ccpp-framework that referenced this issue Jan 11, 2024
…han local name to fix bug (#517)

This PR is a bugfix for ufs-community/ccpp-physics#127

#503 was merged to fix the case when no scheme within a group has an init phase. A bug was introduced described in the ccpp-physics issue above. This fix ensures that the ccpp_t_instance variable is only passed once in the caps.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants