Redundant surface variable cleanup by mzhangw · Pull Request #659 · NCAR/ccpp-physics

mzhangw · 2021-05-17T21:38:00Z

This effort is to address the issue.
Associated PR: NOAA-EMC/ufsatm#317

…_temperature_over_ice

into redund_surface

climbfuji

This is looking good! A couple of changes (mainly reversing whitespace changes), please. Hopefully all regression tests pass on Hera with both compilers.

climbfuji · 2021-05-27T19:17:21Z

           semis_lnd(i) = semis_rad(i)
-             qss_lnd(i) = qss(i)
-            hflx_lnd(i) = hflx(i)
+           qss_lnd(i) = qss(i)


Please revert indentation changes in lines 198 and 199

climbfuji · 2021-05-27T19:17:43Z

           weasd_ice(i) = weasd(i)
-            tsfc_ice(i) = tisfc(i)
-           tsurf_ice(i) = tisfc(i)
+           tsfci(i) = tisfc(i)


Same here, = were aligned previously.

climbfuji · 2021-05-27T19:18:32Z

              zorl(i)  = cice(i) * zorli(i)   + (one - cice(i)) * zorlo(i)
-              tsfc(i)  = tsfc_ice(i) ! over lake (and ocean when uncoupled)
+              tsfc(i)  = tsfci(i) ! over lake (and ocean when uncoupled)
+


Please remove extra blank line

climbfuji · 2021-05-27T19:20:12Z

      subroutine GFS_surface_generic_pre_run (im, levs, vfrac, islmsk, isot, ivegsrc, stype, vtype, slope, &
                          prsik_1, prslk_1, tsfc, phil, con_g,                                             &
-                          sigmaf, soiltyp, vegtype, slopetyp, work3, tsurf, zlvl,                          &
+                          sigmaf, soiltyp, vegtype, slopetyp, work3, zlvl,                                 &


This change has been made in a previous PR (#627 I think), you will see the merge conflict when you update your code.

climbfuji · 2021-05-27T19:21:29Z

          weasd_save(i) = weasd(i)
          snwdph_save(i) = snwdph(i)
-          tsfc_save(i) = tsfc(i)
+!mz          tsfc_save(i) = tsfc(i)


Please delete instead of commenting out if really not needed (did you check the gsd diag 3d test)?

climbfuji · 2021-05-27T19:22:12Z

            weasd(i) = weasd_save(i)
            snwdph(i) = snwdph_save(i)
-            tsfc(i) = tsfc_save(i)
+!mz            tsfc(i) = tsfc_save(i)


Same here, pease delete instead of commenting out if really not needed.

climbfuji

This looks good to me, thanks for updating the formatting. I will combine this PR with other PRs, so that you don't have to keep updating the PRs from main all the time.

climbfuji · 2021-07-23T14:35:51Z

@mzhangw I just discovered that the changes in your PR lead to situations where you are passing the SAME variable (i.e. same standard name) to the scheme twice with different local name. That is forbidden, not just by CCPP but also by Fortran standards. See for example sfc_nst and sfc_drv_ruc. I'll fix that in my effort to bring it up to date with the current code and combine it with Xia's changes.

climbfuji · 2021-07-23T14:47:26Z

I also get those errors from the host model variable definitions:

ERROR: Multiple definitions of standard_name surface_longwave_emissivity_over_ice in type/variable defintions

Will work on it.

mzhangw · 2021-07-23T17:04:27Z

Thanks!

…

On Fri, Jul 23, 2021 at 8:47 AM Dom Heinzeller ***@***.***> wrote: I also get those errors from the host model variable definitions: ERROR: Multiple definitions of standard_name surface_longwave_emissivity_over_ice in type/variable defintions Will work on it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG7TW2T7IZJLYLKWNGRS4W3TZF6IRANCNFSM45BIHLHQ> .

climbfuji · 2021-07-23T17:20:18Z

I actually don't understand how this code compiled, ran and verified against the existing baselines. The metadata parser immediately threw errors about duplicate standard names in several ccpp-physics routines. I don't remember adding this check recently, but maybe I did.

mzhangw · 2021-07-23T17:30:59Z

It did pass all RTs a while ago. I will take a look at it again today if it is ok with you. Sitting at Dr. office now.

…

On Fri, Jul 23, 2021 at 11:20 AM Dom Heinzeller ***@***.***> wrote: I actually don't understand how this code compiled, ran and verified against the existing baselines. The metadata parser immediately threw errors about duplicate standard names in several ccpp-physics routines. I don't remember adding this check recently, but maybe I did. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG7TW2QNZQ2IVEGZ72HLPDLTZGQF3ANCNFSM45BIHLHQ> .

mzhangw · 2021-07-28T15:20:31Z

Recent code update causes some conflicts with this PR. I need revisit this PR.

climbfuji · 2021-07-29T14:53:44Z

Thanks, agreed. The code should never have compiled or run. Passing the same variable twice to a subroutine is dangerous and can have bad consequences. This PR is therefore on hold until the issues are resolved. Thanks for working on it, and sorry for not catching the problems in the first review (GitHub diff view isn't any good for this type of changes).

mzhangw · 2021-07-29T15:08:01Z

The PRs did pass all RTs back in May (See my attached log). However, the surface codes have been updated significantly since then. It won't hurt to revisit it with new updates.

…

On Thu, Jul 29, 2021 at 8:53 AM Dom Heinzeller ***@***.***> wrote: Thanks, agreed. The code should never have compiled or run. Passing the same variable twice to a subroutine is dangerous and can have bad consequences. This PR is therefore on hold until the issues are resolved. Thanks for working on it, and sorry for not catching the problems in the first review (GitHub diff view isn't any good for this type of changes). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#659 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AG7TW2WGQXVXZ7CMUS726IDT2FTQHANCNFSM45BIHLHQ> .

climbfuji · 2021-07-29T15:09:46Z

The PRs did pass all RTs back in May (See my attached log). However, the surface codes have been updated significantly since then. It won't hurt to revisit it with new updates.
…

Even if they passed, the code was incorrect. You are not allowed to pass the same variable (e.g. same standard name) twice to a Fortran function or subroutine. Fortunately, the CCPP framework now catches these errors.

climbfuji · 2021-08-05T20:42:59Z

This PR will be replaced with a new one.

mzhangw and others added 8 commits May 17, 2021 09:46

change surface_skin_temperature_over_ice_interstitial to surface_skin…

8f75b58

…_temperature_over_ice

cleanup the standard/long name of semis_wat/semis_lnd/semis_ice/tsfc_wat

826dca1

Merge branch 'NCAR:main' into redund_surface

a43f331

tsfc_lnd and tsfc_save cleanup

f435ba5

Merge branch 'redund_surface' of https://github.com/mzhangw/ccpp-physics

9d97823

into redund_surface

Merge branch 'NCAR:main' into redund_surface

1977cba

delete turf*, which is not used

8a33f3a

Merge branch 'redund_surface' of https://github.com/mzhangw/ccpp-physics

7c0e99f

into redund_surface

mzhangw mentioned this pull request May 25, 2021

Redundant surface variable cleanup NOAA-EMC/ufsatm#317

Closed

climbfuji added the work in progress label May 27, 2021

mzhangw changed the title ~~Redundant surface variable cleanup (WORK IN PROGRESS)~~ Redundant surface variable cleanup May 27, 2021

mzhangw marked this pull request as ready for review May 27, 2021 18:57

mzhangw requested review from JulieSchramm, climbfuji, grantfirl and llpcarson as code owners May 27, 2021 18:57

climbfuji requested changes May 27, 2021

View reviewed changes

format change as requested

99bac72

climbfuji approved these changes Jul 1, 2021

View reviewed changes

climbfuji self-requested a review July 27, 2021 20:54

climbfuji added bug do not merge labels Jul 29, 2021

climbfuji added wontfix and removed work in progress labels Aug 5, 2021

climbfuji closed this Aug 5, 2021

mzhangw deleted the redund_surface branch December 15, 2021 21:16

Conversation

mzhangw commented May 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

climbfuji left a comment

Choose a reason for hiding this comment

Uh oh!

climbfuji May 27, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji May 27, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji May 27, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji May 27, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji May 27, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji May 27, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji left a comment

Choose a reason for hiding this comment

Uh oh!

climbfuji commented Jul 23, 2021

Uh oh!

climbfuji commented Jul 23, 2021

Uh oh!

mzhangw commented Jul 23, 2021 via email

Uh oh!

climbfuji commented Jul 23, 2021

Uh oh!

mzhangw commented Jul 23, 2021 via email

Uh oh!

mzhangw commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

climbfuji commented Jul 29, 2021

Uh oh!

mzhangw commented Jul 29, 2021 via email

Uh oh!

climbfuji commented Jul 29, 2021

Uh oh!

climbfuji commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mzhangw commented May 17, 2021 •

edited

Loading

mzhangw commented Jul 28, 2021 •

edited

Loading