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

Issue 1984 fix: reduce warnings for ccpp-physics #149

Closed
wants to merge 4 commits into from
Closed

Issue 1984 fix: reduce warnings for ccpp-physics #149

wants to merge 4 commits into from

Conversation

scrasmussen
Copy link

Description of Changes:

Changes to get rid of all the warnings, as pointed out in ufs-weather-model Issue#1984. The fixes for the second and third bullet points were intuited from how similar variables are treated in the code base, if different behavior is preferred please let me know!

  • For all the warning #5462: Global name too long, shortened from warnings: this issue may be already fixed by the Intel compiler, per their message board. I haven't had access to the newer Intel 2024.0.1 or 2024.0.2 versions to test.
  • For varibles zmtb, zlwb, zogw the warning was "unified_ugwp.F90(256): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value." This fix here is they are initialized to 0.0. The following table provides justification for setting the variable to 0, it's how they are always initialized.
file line description
ugwpv1_gsldrag.F90 521 zlwb(:)= 0. ; zogw(:)=0.
ugwp_driver_v0.F 206 zmtb(i) = 0.0
cires_ugwp.F90 297 if (do_ugwp) zlwb(:) = 0.

  • The drain_cpl and dsnow_cpl warnings were "explicit INTENT(OUT) declaration is not given an explicit value". These variables are changed from intent(out) to intent(in) variables. This is to replicate the rain_cpl and snow_cpl variables.

  • The err_message return value of two functions were not defined. The error message is defined as "" and if an error occurs in the open and write statements it will return the error message. Note: this doesn't have the additional errflg, as stated in the coding rules, figure that would be an issue for a different PR if we want it fixed?

  • The last remaining warning message still needs to be fixed. It is FV3/ccpp/physics/physics/dcyc2t3.f(184): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value. [ADJSFCULW]. This is only used in a print statement, so a possible fix is to change it from and intent(out) variable to an intent(inout) variable or remove it? I wasn't sure how to treat this one so would like input if possible, thanks!

Tests Conducted:

Testing on Derecho with Intel Fortran 2023.2.1. Result of testing is as follows:

  • Builds without the warnings described as fixed by this PR. Only seeing "Global name too long" and "ADJSFCULW not given an explicit value" warnings.
  • NOTE: Derecho CMake is doing the final linking step to the ESMF and Parallel IO libraries incorrectly. When I manually run the linking command with the correct paths to those libraries, it links correctly.
  • Working on getting the regression test script running but since it does its own build step first, it isn't working due to the linking issue in the previous bullet point. Will add a comment once the regression test script are running properly.

Dependencies:

None

Documentation:

No changes.

Issue (optional):

Fixes most issues mentioned in ufs-weather-model Issue#1984.

dustinswales
dustinswales approved these changes Jan 2, 2024
View reviewed changes
Copy link
Collaborator

@dustinswales dustinswales left a comment

Choose a reason for hiding this comment

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

@scrasmussen Thanks for making these changes.
Looks good, just wondering if we should remove the unused arguments in GFS_surface_generic_pre, or fix their intent?

physics/GFS_surface_generic_pre.F90 Outdated Show resolved Hide resolved
@scrasmussen
zmtb, zlwb, and zogw are initialized to 0.0 to fix "intent(out) varia…
952d62a 
…bles not

given an explicit value" warning. The following table provides justification for
setting the variable to 0, it is how they are treated in other places.

| file               | line | description                 |
|--------------------+------+-----------------------------|
| ugwpv1_gsldrag.F90 |  521 | zlwb(:)= 0. ; zogw(:)=0.    |
| ugwp_driver_v0.F   |  206 | zmtb(i)     = 0.0           |
| cires_ugwp.F90     |  297 | if (do_ugwp) zlwb(:)   = 0. |
@scrasmussen
issue 1984 fix: "intent out variables not given a value" warning. The…
f183508 
… drain_cpl and dsnow_cpl vars are changed from intent(out) to intent(in) variables. This is to replicate the rain_cpl and snow_cpl variables.
@scrasmussen
issue 1984 fix: initialize err_message to "" because they are intent(…
ba7c627 
…out) variables. Variable err_message will report any errors in open and read statements
@scrasmussen scrasmussen marked this pull request as draft January 10, 2024 21:39
@scrasmussen scrasmussen marked this pull request as ready for review January 11, 2024 20:37
@grantfirl
Copy link
Collaborator

@scrasmussen Regarding your comment about ADJSFCULW, I would just remove the variable from the argument list (in both the Fortran and the metadata), since even the write statement is commented out.

@grantfirl
Copy link
Collaborator

@scrasmussen @dustinswales Regarding the err_message coming out of load_o3prog, this subroutine is not being called from the physics (and therefore I guess doesn't need to follow the [errflg, errmsg] protocol) but from the host in GFS_typedefs. However, it appears that nothing is done to stop the model if it errors out. Should something be added in GFS_typedefs to stop the model and print out the message if the open or read statements error out? err_message looks to be ignored upon return.

@dustinswales
Copy link
Collaborator

@scrasmussen @dustinswales Regarding the err_message coming out of load_o3prog, this subroutine is not being called from the physics (and therefore I guess doesn't need to follow the [errflg, errmsg] protocol) but from the host in GFS_typedefs. However, it appears that nothing is done to stop the model if it errors out. Should something be added in GFS_typedefs to stop the model and print out the message if the open or read statements error out? err_message looks to be ignored upon return.

@grantfirl When I refactored I didn't add any error handling, and there wasn't any to start. You're 100% correct that there should be something here, at least to return an error if the file can't be found in load_o3prog.

@DusanJovic-NOAA
Copy link

It would be nice to fix this compiler error:

 /scratch2/NCEPDEV/fv3-cam/Dusan.Jovic/ufs/develop/ufs-weather-model/FV3/ccpp/physics/physics/GWD/cires_ugwpv1_triggers.F90:301:30:

  301 |           if (dtot >= tlim_fgf ) kex = kex+1
      |                              1
Error: Symbol 'tlim_fgf' at (1) has no IMPLICIT type; did you mean 'tlim_okw'?                                                                                                               
make[2]: *** [FV3/ccpp/physics/CMakeFiles/ccpp_physics.dir/physics/GWD/cires_ugwpv1_triggers.F90.o] Error 1
make[1]: *** [FV3/ccpp/physics/CMakeFiles/ccpp_physics.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....

which occurs when compiling code with flag that enforces implicit none (for the gnu compiler -fimplicit-none)

@grantfirl grantfirl mentioned this pull request Feb 7, 2024
Merged
@grantfirl
Copy link
Collaborator

@scrasmussen I talked to @Qingfu-Liu, and he has a few more things to add to this PR before we're ready to merge. @Qingfu-Liu Let us know how this progresses. I'd like to combine this with #150 and #168 if possible, and those are ready to merge. If we need more time for this one, we can just merge #150 and #168 together and do this one separately.

@Qingfu-Liu
Copy link
Collaborator

@scrasmussen I talked to @Qingfu-Liu, and he has a few more things to add to this PR before we're ready to merge. @Qingfu-Liu Let us know how this progresses. I'd like to combine this with #150 and #168 if possible, and those are ready to merge. If we need more time for this one, we can just merge #150 and #168 together and do this one separately.

@grantfirl I am not sure if I should create a new PR or wait for @scrasmussen to update this PR? I think we can't update the code changes for this PR (I remember you try to update my PR, and can't do it).

@grantfirl
Copy link
Collaborator

@scrasmussen I talked to @Qingfu-Liu, and he has a few more things to add to this PR before we're ready to merge. @Qingfu-Liu Let us know how this progresses. I'd like to combine this with #150 and #168 if possible, and those are ready to merge. If we need more time for this one, we can just merge #150 and #168 together and do this one separately.

@grantfirl I am not sure if I should create a new PR or wait for @scrasmussen to update this PR? I think we can't update the code changes for this PR (I remember you try to update my PR, and can't do it).

@Qingfu-Liu You can create a branch from this one, put your changes on top of it, and then put a pull request into this one for @scrasmussen to approve/merge.

git remote add soren-fork https://github.com/scrasmussen/ccpp-physics
git fetch soren-fork
git checkout ufs/issue-1984-fix-warnings
git checkout -b qingfus_changes
Make your changes
git add [your changes]
git commit -m [your changes]
git push [to your fork]
Use the GitHub interface to start a PR from qingfus_changes branch into @scrasmussen's fork targeting the ufs/issue-1984-fix-warnings branch.

Soren and I can review your changes. Once approved/merged, your changes will show up in this PR.

@Qingfu-Liu
Copy link
Collaborator

Qingfu-Liu commented Feb 9, 2024 via email

@grantfirl
Copy link
Collaborator

@Qingfu-Liu @scrasmussen This morning, @DusanJovic-NOAA mentioned that he is removing the compiler flag this is hiding all warnings, so, for getting rid of all warnings in the future, we shouldn't have to compile manually without the -no warn flag. FYI.

@Qingfu-Liu
Copy link
Collaborator

@grantfirl
Thank you very much. I create a new PR#169 by combine the PR#149 with my new changes. I am not sure how to merge my changes back to the PR#149.

@grantfirl grantfirl mentioned this pull request Feb 20, 2024
Merged
@grantfirl
Copy link
Collaborator

Closed in favor of #169

@grantfirl grantfirl closed this Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdtoyNOAA mdtoyNOAA mdtoyNOAA approved these changes

@Qingfu-Liu Qingfu-Liu Qingfu-Liu approved these changes

@dustinswales dustinswales dustinswales approved these changes

@grantfirl grantfirl Awaiting requested review from grantfirl grantfirl is a code owner

@mzhangw mzhangw Awaiting requested review from mzhangw

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

Successfully merging this pull request may close these issues.
6 participants
@scrasmussen @grantfirl @dustinswales @DusanJovic-NOAA @Qingfu-Liu @mdtoyNOAA