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

Bug fix for z0t #2

Merged
merged 2 commits into from
Nov 29, 2022
Merged

Conversation

ChunxiZhang-NOAA
Copy link

This is a corresponding PR to ccpp-physics PR#23. It fixed a bug related to the definition of z0t. A new variable ztmax is added.

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA It seems that action is failing because GitHub Action runner is using new version of OS and spack package installer is failing. I am looking into it. I could also test your changes manually under UFS. BTW, it might be nice to add this new variable to the history file. You just need it to add just after this line,

call fld_add("pblh" ,"height of pbl" ,"m" ,ptr2d=ptr2d, v1r8=noahmp%model%pblh)
. Of course, this also changes the BLs. So, if you already tested and there is no BL change then I could do it later.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu I added the variable to the history file as you suggested. I tested it in the UFS, and it changed the BLs.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu @barlage Please review this PR ASAP. This PR is scheduled to be committed today.

Copy link
Collaborator

@barlage barlage left a comment

Choose a reason for hiding this comment

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

These changes look good to me.

@ChunxiZhang-NOAA
Copy link
Author

@barlage Thank you!

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA I am getting following errors from container with GNU compiler,

/test/app/noahmp/src/module_sf_noahmplsm.f90:1:0:

    1 | #define CCPP
      | 
Warning: "CCPP" redefined
<command-line>:1:0:

note: this is the location of the previous definition
/test/app/noahmp/src/module_sf_noahmplsm.f90:2137:132:

 2137 |          l_rsurf = (-zsoil(1)) * ( exp ( (1.0 - min(1.0,sh2o(1)/parameters%smcmax(1))) ** parameters%rsurf_exp ) - 1.0 ) / ( 2.71828 - 1.0 )
      |                                                                                                                                    1
Error: Line truncated at (1) [-Werror=line-truncation]
/test/app/noahmp/src/module_sf_noahmplsm.f90:2137:132:

 2137 |          l_rsurf = (-zsoil(1)) * ( exp ( (1.0 - min(1.0,sh2o(1)/parameters%smcmax(1))) ** parameters%rsurf_exp ) - 1.0 ) / ( 2.71828 - 1.0 )
      |                                                                                                                                    1
Error: Expected a right parenthesis in expression at (1)
/test/app/noahmp/src/module_sf_noahmplsm.f90:2138:132:

 2138 |          d_rsurf = 2.2e-5 * parameters%smcmax(1) * parameters%smcmax(1) * ( 1.0 - parameters%smcwlt(1) / parameters%smcmax(1) ) ** (2.0+3.0/parameters%bexp(1))
      |                                                                                                                                    1
Error: Line truncated at (1) [-Werror=line-truncation]
/test/app/noahmp/src/module_sf_noahmplsm.f90:2138:132:

 2138 |          d_rsurf = 2.2e-5 * parameters%smcmax(1) * parameters%smcmax(1) * ( 1.0 - parameters%smcwlt(1) / parameters%smcmax(1) ) ** (2.0+3.0/parameters%bexp(1))
      |                                                                                                                                    1
Error: Syntax error in expression at (1)
/test/app/noahmp/src/module_sf_noahmplsm.f90:8869:132:

 8869 |                    smceqdeep = parameters%smcmax(nsoil) * ( -parameters%psisat(nsoil) / ( -parameters%psisat(nsoil) - dzsnso(nsoil) ) ) ** (1./parameters%bexp(nsoil))
      |                                                                                                                                    1
Error: Line truncated at (1) [-Werror=line-truncation]
/test/app/noahmp/src/module_sf_noahmplsm.f90:8869:132:

 8869 |                    smceqdeep = parameters%smcmax(nsoil) * ( -parameters%psisat(nsoil) / ( -parameters%psisat(nsoil) - dzsnso(nsoil) ) ) ** (1./parameters%bexp(nsoil))
      |                                                                                                                                    1
Error: Expected a right parenthesis in expression at (1)
/test/app/noahmp/src/module_sf_noahmplsm.f90:8881:132:

 8881 |            smceqdeep = parameters%smcmax(nsoil) * ( -parameters%psisat(nsoil) / ( -parameters%psisat(nsoil) - dzsnso(nsoil) ) ) ** (1./parameters%bexp(nsoil))
      |                                                                                                                                    1
Error: Line truncated at (1) [-Werror=line-truncation]
/test/app/noahmp/src/module_sf_noahmplsm.f90:8881:132:

 8881 |            smceqdeep = parameters%smcmax(nsoil) * ( -parameters%psisat(nsoil) / ( -parameters%psisat(nsoil) - dzsnso(nsoil) ) ) ** (1./parameters%bexp(nsoil))
      |                                                                                                                                    1
Error: Syntax error in expression at (1)
f951: some warnings being treated as errors
make[2]: *** [CMakeFiles/noahmp.dir/build.make:283: CMakeFiles/noahmp.dir/src/module_sf_noahmplsm.f90.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/noahmp.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

@barlage
Copy link
Collaborator

barlage commented Nov 28, 2022

Does GNU have a column 132 truncation requirement?

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu It looks the issue is not related to this PR. The error message shows the truncation error. If can be solved with compiling options.

@ChunxiZhang-NOAA
Copy link
Author

Fortran90+ has 132 line length limit. Intel compiler usually doesn't strictly force it. Looks like gnu does. Maybe it will help by adding this compiler option: -ffree-line-length-none

@barlage
Copy link
Collaborator

barlage commented Nov 28, 2022

In a future update, we can break this line.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu Could you try the compiler option -ffree-line-length-none? I believe it will resolve the issue with this option.

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA I compiled with -DCMAKE_Fortran_FLAGS="-ffree-line-length-none -ffixed-line-length-none" and those error disappears (I think if you could you could fix those lines and we don't need to worry about the compiler flags, this also requires some changes in GitHub action that I fixed later if we need to use those flags) but following came up

Scanning dependencies of target noahmp
[  5%] Building Fortran object CMakeFiles/noahmp.dir/drivers/ccpp/machine.F.o
/test/app/noahmp/drivers/ccpp/machine.F:9:47:

    9 |       integer, parameter :: kind_sngl_prec = 4                          &
      |                                               1
Error: Syntax error in data declaration at (1)
/test/app/noahmp/drivers/ccpp/machine.F:24:53:

   24 |       integer, parameter :: kind_phys = kind_dbl_prec
      |                                                     1
Error: Symbol 'kind_dbl_prec' at (1) has no IMPLICIT type
/test/app/noahmp/drivers/ccpp/machine.F:29:48:

   29 |       integer, parameter :: kind_io8 = kind_phys                        &! Note kind_io8 is not always 8 bytes
      |                                                1
Error: Symbol 'kind_phys' at (1) has no IMPLICIT type; did you mean 'kind_io8'?
/test/app/noahmp/drivers/ccpp/machine.F:35:53:

   35 |       integer, parameter :: kind_dyn  = kind_dbl_prec
      |                                                     1
Error: Symbol 'kind_dbl_prec' at (1) has no IMPLICIT type
/test/app/noahmp/drivers/ccpp/machine.F:39:25:

   39 |       real(kind=kind_phys), parameter :: mprec = 1.e-12           ! machine precision to restrict dep
      |                         1
Error: Symbol 'kind_phys' at (1) has no IMPLICIT type
/test/app/noahmp/drivers/ccpp/machine.F:40:25:

   40 |       real(kind=kind_phys), parameter :: grib_undef = 9.99e20     ! grib undefine value
      |                         1
Error: Symbol 'kind_phys' at (1) has no IMPLICIT type
make[2]: *** [CMakeFiles/noahmp.dir/build.make:205: CMakeFiles/noahmp.dir/drivers/ccpp/machine.F.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/noahmp.dir/all] Error 2
make: *** [Makefile:136: all] Error 2

@barlage
Copy link
Collaborator

barlage commented Nov 28, 2022

@uturuncoglu any reason why these are showing up now? as far as I know, none of this code has been changed in this PR.

@uturuncoglu
Copy link
Collaborator

@barlage i am not sure. Maybe adding -DCMAKE_Fortran_FLAGS="-ffree-line-length-none -ffixed-line-length-none" to compile triggered these. I think it would be nice to fix long lines and try again without those extra flags.

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA @barlage Okay. I fixed the long lines and compile without extra flags. It builds without any issue and also runs under my Docker container. So, we just need to fix those long lines and split them using &.

@barlage
Copy link
Collaborator

barlage commented Nov 28, 2022

I guess the question also applies to the original error since this long line is in the original version too.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu Did you try to only use -ffree-line-length-none?

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA it seems it is building fine with only this option.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu Good to know. I think it is because -ffixed-line-length-none makes the "&" character at column 73 legal but "&" in the context is illegal in Fortran77.

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA Thanks. I think this is fine. I could make changes in the action once I fix the issue with the updated runner.

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA I could merge it now, if it is fine for you.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu The UFS commit process for PR#1394 has started one hour ago. It would be great if you can create a new PR for the changes in the actions.

@uturuncoglu
Copy link
Collaborator

@ChunxiZhang-NOAA Yes. No worries about action. I could fixed later.

@ChunxiZhang-NOAA
Copy link
Author

@uturuncoglu Ok, thank you!

@jkbk2004
Copy link
Collaborator

All tests are done on ufs-wm pr #1493 side. Can we start merging in this pr?

@uturuncoglu uturuncoglu merged commit 9c8a1c0 into NOAA-EMC:develop Nov 29, 2022
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.

4 participants