Sm nov222021#824
Conversation
|
@JongilHan66 @HelinWei-NOAA could you please review this PR ? |
tanyasmirnova
left a comment
tanyasmirnova
left a comment
There was a problem hiding this comment.
Most changes seem to be just cosmetic. I do not have any problems with these changes.
| tsurf_ice(i) = tisfc(i) | ||
| ep1d_ice(i) = zero | ||
| gflx_ice(i) = zero | ||
| zorli(i) = max(1.0e-5, min(one, zorli(i))) |
There was a problem hiding this comment.
@JongilHan66 @HelinWei-NOAA @tanyasmirnova This is the change where zorli gets its limits.
| uustar_wat(i) = uustar(i) | ||
| tsfc_wat(i) = tsfco(i) | ||
| tsurf_wat(i) = tsfco(i) | ||
| zorlo(i) = max(1.0e-5, min(one, zorlo(i))) |
There was a problem hiding this comment.
zorlo gets its limits here
JongilHan66
left a comment
JongilHan66
left a comment
There was a problem hiding this comment.
The changes look ok for me.
grantfirl
left a comment
grantfirl
left a comment
There was a problem hiding this comment.
Thanks @SMoorthi-emc. Looks good to me.
|
High UFS commit queue priority due to this being needed for P8. |
|
@SMoorthi-emc Shall I go ahead and submit upstream PRs to fv3atm and ufs-weather-model? Also, given the nature of these changes, I'm guessing that regression test baselines will need to be updated? |
|
Grant, no, I will submit that PR this evening.
I have some fix in FV3GFS_io.F90 that fixes some error related to initializing snow over ice.
Thanks
Moorthi
…Sent from my iPhone
On Jan 4, 2022, at 4:44 PM, Grant Firl ***@***.***> wrote:
@SMoorthi-emc Shall I go ahead and submit upstream PRs to fv3atm and ufs-weather-model? Also, given the nature of these changes, I'm guessing that regression test baselines will need to be updated?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
You are receiving this because you were mentioned.
|
|
Sam,
The above loop was added by me a long time ago when we did not have
"snod_lnd" , "snod_ice" etc. not part of the restart file.
Now that those variables are already there this loop actually can cause
error, specifically for the uncoupled case as it goes through the loo all
the time.
Moorthi
…On Tue, Jan 4, 2022 at 3:29 PM Samuel Trahan (NOAA contractor) < ***@***.***> wrote:
***@***.**** commented on this pull request.
As noted elsewhere, I'd like another developer with knowledge of the
surface schemes to confirm it is okay to remove the block of code from
GFS_surface_composites.F90
The shear quantity of whitespace changes is indulgent, but I would not
deny you the satisfaction of a job well done. The code does look cleaner
too. I just hope another developer doesn't PR a new indentation for those
lines.
------------------------------
In physics/GFS_surface_composites.F90
<#824 (comment)>:
> - if (.not. cplflx .or. kdt == 1) then
- if (frac_grid) then
- do i=1,im
- if (dry(i)) then
- if (icy(i)) then
- tem = one / (cice(i)*(one-frland(i)))
- snowd_ice(i) = max(zero, (snowd(i) - snowd_lnd(i)*frland(i)) * tem)
- weasd_ice(i) = max(zero, (weasd(i) - weasd_lnd(i)*frland(i)) * tem)
- endif
- elseif (icy(i)) then
- tem = one / cice(i)
- snowd_lnd(i) = zero
- snowd_ice(i) = snowd(i) * tem
- weasd_lnd(i) = zero
- weasd_ice(i) = weasd(i) * tem
- endif
- enddo
- else
- do i=1,im
- if (dry(i)) then
- snowd_lnd(i) = snowd(i)
- weasd_lnd(i) = weasd(i)
- snowd_ice(i) = zero
- weasd_ice(i) = zero
- elseif (icy(i)) then
- snowd_lnd(i) = zero
- weasd_lnd(i) = zero
- tem = one / cice(i)
- snowd_ice(i) = snowd(i) * tem
- weasd_ice(i) = weasd(i) * tem
- endif
- enddo
- endif
- endif
-
-! write(0,*)' minmax of ice snow=',minval(snowd_ice),maxval(snowd_ice)
-
I'd like another physics scheme developer with knowledge of this to
confirm this change is correct before the PR is merged.
—
Reply to this email directly, view it on GitHub
<#824 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALLVRYWO4DPYABUI3XSEHCLUUNKD7ANCNFSM5LIEIGFA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Dr. Shrinivas Moorthi
Research Meteorologist
Modeling and Data Assimilation Branch
Environmental Modeling Center / National Centers for Environmental
Prediction
5830 University Research Court - (W/NP23), College Park MD 20740 USA
Tel: (301)683-3718
e-mail: ***@***.***
Phone: (301) 683-3718 Fax: (301) 683-3718
|
|
Helin, you are the point of contact for sfc_cice.f For sea ice, snow
depth is specified there that is imported from CICE6.
For the uncoupled case snow depth over ice is computed in sfc_sice.f.
Moorthi
…On Tue, Jan 4, 2022 at 5:52 PM HelinWei-NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In physics/GFS_surface_composites.F90
<#824 (comment)>:
> - if (.not. cplflx .or. kdt == 1) then
- if (frac_grid) then
- do i=1,im
- if (dry(i)) then
- if (icy(i)) then
- tem = one / (cice(i)*(one-frland(i)))
- snowd_ice(i) = max(zero, (snowd(i) - snowd_lnd(i)*frland(i)) * tem)
- weasd_ice(i) = max(zero, (weasd(i) - weasd_lnd(i)*frland(i)) * tem)
- endif
- elseif (icy(i)) then
- tem = one / cice(i)
- snowd_lnd(i) = zero
- snowd_ice(i) = snowd(i) * tem
- weasd_lnd(i) = zero
- weasd_ice(i) = weasd(i) * tem
- endif
- enddo
- else
- do i=1,im
- if (dry(i)) then
- snowd_lnd(i) = snowd(i)
- weasd_lnd(i) = weasd(i)
- snowd_ice(i) = zero
- weasd_ice(i) = zero
- elseif (icy(i)) then
- snowd_lnd(i) = zero
- weasd_lnd(i) = zero
- tem = one / cice(i)
- snowd_ice(i) = snowd(i) * tem
- weasd_ice(i) = weasd(i) * tem
- endif
- enddo
- endif
- endif
-
-! write(0,*)' minmax of ice snow=',minval(snowd_ice),maxval(snowd_ice)
-
@SMoorthi-emc <https://github.com/SMoorthi-emc> snowd/weasd over
different parts of fraction grid are still used for composition in
GFS_surface_composites_post_run. Only RUC lsm can provide them after this
removal. Why do you want to remove them?
—
Reply to this email directly, view it on GitHub
<#824 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALLVRYRPVJGRYFLLXEPA3T3UUN24FANCNFSM5LIEIGFA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Dr. Shrinivas Moorthi
Research Meteorologist
Modeling and Data Assimilation Branch
Environmental Modeling Center / National Centers for Environmental
Prediction
5830 University Research Court - (W/NP23), College Park MD 20740 USA
Tel: (301)683-3718
e-mail: ***@***.***
Phone: (301) 683-3718 Fax: (301) 683-3718
|
|
Helin, it is already initialized there, but there is an error that I am
correcting.
Moorthi
…On Tue, Jan 4, 2022 at 5:58 PM HelinWei-NOAA ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In physics/GFS_surface_composites.F90
<#824 (comment)>:
> - if (.not. cplflx .or. kdt == 1) then
- if (frac_grid) then
- do i=1,im
- if (dry(i)) then
- if (icy(i)) then
- tem = one / (cice(i)*(one-frland(i)))
- snowd_ice(i) = max(zero, (snowd(i) - snowd_lnd(i)*frland(i)) * tem)
- weasd_ice(i) = max(zero, (weasd(i) - weasd_lnd(i)*frland(i)) * tem)
- endif
- elseif (icy(i)) then
- tem = one / cice(i)
- snowd_lnd(i) = zero
- snowd_ice(i) = snowd(i) * tem
- weasd_lnd(i) = zero
- weasd_ice(i) = weasd(i) * tem
- endif
- enddo
- else
- do i=1,im
- if (dry(i)) then
- snowd_lnd(i) = snowd(i)
- weasd_lnd(i) = weasd(i)
- snowd_ice(i) = zero
- weasd_ice(i) = zero
- elseif (icy(i)) then
- snowd_lnd(i) = zero
- weasd_lnd(i) = zero
- tem = one / cice(i)
- snowd_ice(i) = snowd(i) * tem
- weasd_ice(i) = weasd(i) * tem
- endif
- enddo
- endif
- endif
-
-! write(0,*)' minmax of ice snow=',minval(snowd_ice),maxval(snowd_ice)
-
@SMoorthi-emc <https://github.com/SMoorthi-emc> Just saw your response to
Grant, you are going to initialize them in FV3GFS_io.F90 rather than here?
—
Reply to this email directly, view it on GitHub
<#824 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALLVRYQMINJPHOMZZ7QO5X3UUN3SBANCNFSM5LIEIGFA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Dr. Shrinivas Moorthi
Research Meteorologist
Modeling and Data Assimilation Branch
Environmental Modeling Center / National Centers for Environmental
Prediction
5830 University Research Court - (W/NP23), College Park MD 20740 USA
Tel: (301)683-3718
e-mail: ***@***.***
Phone: (301) 683-3718 Fax: (301) 683-3718
|
|
All, I had to modify "GFS_surface_composites.F90" to take care of a crash in NoahMP. |
| if (kdt == 1 .or. (.not. cplflx .or. lakefrac(i) > zero)) then | ||
| tem = one / (cice(i)*(one-frland(i))) | ||
| snowd_ice(i) = max(zero, (snowd(i) - snowd_lnd(i)*frland(i)) * tem) | ||
| weasd_ice(i) = max(zero, (weasd(i) - weasd_lnd(i)*frland(i)) * tem) |
There was a problem hiding this comment.
Moorthi, with RUC LSM snowd_ice and weasd_ice should not be recomputed here because they are computed internally. I think we have to add to the condition in brackets: lsm /= lsm_ruc.
There was a problem hiding this comment.
Tanya,
I think all LSM's are called after calling this routine. As long as grid mean values are computed correctly, there should be no issue. When the coupled model is used, it is different. This loop takes care of the case when there is a new icy points if they happen to be defined within this routine.
Moorthi
There was a problem hiding this comment.
Moorthi, the snow depth on the land and ice fractions could be very different, and because RUC LSM computes both weasd_ice and weasd_lnd, it wants to carry them along separately. Therefore, they should not be recomputed here from the grid mean when RUC LSM is used. The grid mean snow depth could be used as the diagnostics variable, convenient for plotting.
There was a problem hiding this comment.
I originally wanted to do this computation on only new icy points, but unfortunately, there is no memory of previous "icy" points. I should also point out that "sfc_sice.f" also computes ice point variables, just like RUC does. While the computation here is not necessary for the icy points that remain icy, it does not hurt.
There was a problem hiding this comment.
Again, this is not special to RUC as what you stated is true for non-RUC also as in those cases ice is computed in sfc_sice.f
Well, I welcome anyone to come up with better alternative.
I should point out that this is already happening in the current code- so this should not be a point for rejecting my update as it improves on what currently exists.
| endif | ||
| elseif (icy(i)) then | ||
| if (kdt == 1 .or. (.not. cplflx .or. lakefrac(i) > zero)) then | ||
| tem = one / cice(i) |
There was a problem hiding this comment.
Same here, with RUC LSM it should not be done.
There was a problem hiding this comment.
I just disagree.
7 participants
This PR updates "GFS_surface_composites.F90" and the associated "meta" file to remove unnecessary (and erroneous code).
It also adds a limit on zorli for points with ice.
There are some other cosmetic changes in some other routines (primarily blanks)