Cleanup work for adding tiice to restart files for runs without fractional grid

climbfuji commented 3 years ago

Description

When https://github.com/NOAA-EMC/fv3atm/pull/212 was merged, it was decided that always storing the ice layer temperatures separately from the soil layer temperatures is cleaner and less confusing. Until then, they were only stored separately when the fractional grid was used, but combined into the soil layer temperature array when the binary grid was used.

Solution

The cleanup work includes:

remove the commented lines around adding tiice to the restart data in io/FV3GFS_io.F90
merge the register_restart_field calls for tiice with the calls for the other arrays above in io/FV3GFS_io.F90
in ccpp-physics, remove the code in GFS_surface_composites.F90 that stores tiice in slt

Testing:

This change will require new baselines (because the slt array in the restart and diagnostic files will be changed)

ShanSunNOAA commented 3 years ago

Impressive bookkeeping - thanks for the info!

junwang-noaa commented 3 years ago

Some issues come up in regional LAM parallel because the tiice was added to restart sfc file in PR#212. LAM runs on non-fractional grid and does not need to have this 2 layers tiice array. Below is the description below from Eric Rogers.

In the version of chgres_cube we're running in the LAM parallels:

commit 19942e0a15ac48a9c45bf1301dff7c837158e556 (HEAD -> develop, origin/develop, origin/HEAD) Date: Tue Dec 15 14:44:02 2020 -0500

these are the dimensions in the sfc_data file coming out of chgres_cube:

netcdf sfc_data_new { dimensions: xaxis_1 = 1754 ; yaxis_1 = 1044 ; zaxis_1 = 4 ; Time = 1 ;

When I ran the LAMDA with the latest develop branch with has the regional inline post, I see an extra dimension added in the sfc_data file that comes out of the model:

netcdf sfc_data_new { dimensions: xaxis_1 = 1754 ; yaxis_1 = 1044 ; zaxis_1 = 2 ; zaxis_2 = 4 ; Time = 1 ;

This caused an abort in one of the codes Tom wrote that runs after the forecast (to address the boundary imbalance problem in the LAM DA runs), since it read the sfc_data file from the model and expected 4 dimensions. Changing this code to account for the extra dimension was trivial, and I was able to get a successful end-to-end run of the LAMDA with the inline post, but this difference may cause some confusion.

@climbfuji Please keep Eric Rogers included when the issue is resolved so that Eric can check if the tiice issue is gone in LAM.

climbfuji commented 3 years ago

Just to make sure we understand each other, tiice should remain in the output for both binary and fractional grid applications. If it causes a problem in one of Tom's codes, then that one will need to be fixed.

junwang-noaa commented 3 years ago

Maybe we misunderstood eath other. Can you remind me why tiice is required for restart for non-fractional grid runs?

On Tue, Mar 2, 2021 at 3:34 PM Dom Heinzeller notifications@github.com wrote:

Just to make sure we understand each other, tiice should remain in the output for both binary and fractional grid applications. If it causes a problem in one of Tom's codes, then that one will need to be fixed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789196987, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI7D6TPTK3QHDUKZUK4KCCTTBVDU3ANCNFSM4U26VHMA .

climbfuji commented 3 years ago

Putting tiice into tsfc, somewhere buried in the physics has several serious implications.

extremely confusing, no developer knows what is going on, where the data is stored (hidden) and retrieved from
this kind of logic deals with host model limitations (or external code limitations, better to say) and does not belong in physics routines
if kice == number of ice levels == klsm == number of lsm levels, then the array is at least filled properly
if kice < klsm, you need to decide what to put into the layers kice+1:klsm - missing value? zero? yet another thing to get confused about
if kice > klsm, the code breaks - and in fact, kice ==9 for RUC LSM, for example --> putting them back into stc will break RUC lsm and any other potential ice model with kice > klsm

ShanSunNOAA commented 3 years ago

tiice was introduced for the fractional grid, where it was for internal ice temp. In the nonfractional case, internal ice temperature borrows the soil temperature slot, as ice and soil won't co-exist in this case.

I thought tiice would store its value in the soil temperature slot in the nonfractioanl case prior writing to restart. In other words, tiice should be purely internal for nonfractional case. Will check when hera is back.

Thanks, Shan

On Tue, Mar 2, 2021 at 1:38 PM Jun Wang notifications@github.com wrote:

Maybe we misunderstood eath other. Can you remind me why tiice is required for restart for non-fractional grid runs?

On Tue, Mar 2, 2021 at 3:34 PM Dom Heinzeller notifications@github.com wrote:

Just to make sure we understand each other, tiice should remain in the output for both binary and fractional grid applications. If it causes a problem in one of Tom's codes, then that one will need to be fixed.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789196987, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AI7D6TPTK3QHDUKZUK4KCCTTBVDU3ANCNFSM4U26VHMA

.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789199431, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALORMVVCK4TZ4EN5NTDA7XDTBVEEJANCNFSM4U26VHMA .

climbfuji commented 3 years ago

This is all confusing ... no matter what, if kice > klsm it will break.

climbfuji commented 3 years ago

tiice is always written to the restart files (and read from it), no matter whether fractional or not. Simply because of the flaws of this design choice that I mentioned above. See current FV3/io/FV3GFS_io.F90.

climbfuji commented 3 years ago

https://github.com/NOAA-EMC/fv3atm/blob/4908898cdd71a84cef482a7e6c330697a39c7378/io/FV3GFS_io.F90#L908

junwang-noaa commented 3 years ago

So tiice is not saved in tsfc any more even for the situation of non-fractional grid with kice<klsm. Then the downstream jobs may need to know this change and update accordingly.

climbfuji commented 3 years ago

So tiice is not saved in tsfc any more even for the situation of non-fractional grid with kice<klsm. Then the downstream jobs may need to know this change and update accordingly.

Yep, seems to be the cleanest solution.

SMoorthi-emc commented 3 years ago

I thought I modified to write tiice separately to avoid these issues.

Sent from my iPhone

On Mar 2, 2021, at 3:49 PM, Dom Heinzeller notifications@github.com wrote:

This is all confusing ... no matter what, if kice > klsm it will break.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

SMoorthi-emc commented 3 years ago

I thought that is what I did. Moorthi

Sent from my iPhone

On Mar 2, 2021, at 3:51 PM, Dom Heinzeller notifications@github.com wrote:

tiice is always written to the restart files (and read from it), no matter whether fractional or not. Simply because of the flaws of this design choice that I mentioned above. See current FV3/io/FV3GFS_io.F90.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

yangfanglin commented 3 years ago

For the GSM/GFS, tsfc has always been used to represent skin temperatures over all surface types/grids, including land, ice and water. Has this been changed ? There are many downstream users relying on this product.

On Tue, Mar 2, 2021 at 4:23 PM SMoorthi-emc notifications@github.com wrote:

I thought that is what I did. Moorthi

Sent from my iPhone

On Mar 2, 2021, at 3:51 PM, Dom Heinzeller notifications@github.com wrote:

tiice is always written to the restart files (and read from it), no matter whether fractional or not. Simply because of the flaws of this design choice that I mentioned above. See current FV3/io/FV3GFS_io.F90.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789226087, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKY5N2PCZX5KGOFD7CHL2GTTBVJNPANCNFSM4U26VHMA .

-- Fanglin Yang, Ph.D. Chief, Model Physics Group Modeling and Data Assimilation Branch

NOAA/NWS/NCEP Environmental Modeling Center

https://www.emc.ncep.noaa.gov/gmb/wx24fy/fyang/ https://www.emc.ncep.noaa.gov/gmb/wx24fy/fyang/

SMoorthi-emc commented 3 years ago

Tsfc has nothing to do with tiice. They are different. Tiice is only in restart (I can’t see the code now - I will confirm later) Moorthi

Sent from my iPhone

On Mar 2, 2021, at 4:34 PM, Fanglin Yang notifications@github.com wrote:

For the GSM/GFS, tsfc has always been used to represent skin temperatures over all surface types/grids, including land, ice and water. Has this been changed ? There are many downstream users relying on this product.

On Tue, Mar 2, 2021 at 4:23 PM SMoorthi-emc notifications@github.com wrote:

I thought that is what I did. Moorthi

Sent from my iPhone

On Mar 2, 2021, at 3:51 PM, Dom Heinzeller notifications@github.com wrote:

tiice is always written to the restart files (and read from it), no matter whether fractional or not. Simply because of the flaws of this design choice that I mentioned above. See current FV3/io/FV3GFS_io.F90.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789226087, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKY5N2PCZX5KGOFD7CHL2GTTBVJNPANCNFSM4U26VHMA .

-- Fanglin Yang, Ph.D. Chief, Model Physics Group Modeling and Data Assimilation Branch

NOAA/NWS/NCEP Environmental Modeling Center

https://www.emc.ncep.noaa.gov/gmb/wx24fy/fyang/ https://www.emc.ncep.noaa.gov/gmb/wx24fy/fyang/ — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

climbfuji commented 3 years ago

I thought I modified to write tiice separately to avoid these issues. …

Yes, that is what you did and what should remain. The cleanup work here was just a follow-up, not a reversal but got misinterpreted.

@yangfanglin this is about tiice and stc, not tsfc.

junwang-noaa commented 3 years ago

I think the impact is that model used to output internal ice temp in stc (soil temp) on ice points, but now the internal ice temp is saved in its own array, I believe the stc does not contain the ice temp any more(restart non-reproducible for non-fractional grid uncoupled with tiice) . At this time, we also need to update the GFS_diagnostics.F90 to allow it output to model history files for non-fractional grid runs (e.g. LAM). The tsfc does not change for non-fractional grid, but is the composite temp for fractional grid.

On Tue, Mar 2, 2021 at 4:40 PM Dom Heinzeller notifications@github.com wrote:

I thought I modified to write tiice separately to avoid these issues. … <#m-3481226871809676753>

Yes, that is what you did and what should remain. The cleanup work here was just a follow-up, not a reversal but got misinterpreted.

@yangfanglin https://github.com/yangfanglin this is about tiice and stc, not tsfc.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789239373, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI7D6TP7CYGRBQUNS4572VLTBVLN5ANCNFSM4U26VHMA .

SMoorthi-emc commented 3 years ago

There is tiice in non fractional grid also. It may also be in stc for non fractional case. I did not add tiice to history partly because I don’t know enough about it plus also did not want change history Moorthi

Sent from my iPhone

On Mar 2, 2021, at 4:55 PM, Jun Wang notifications@github.com wrote:

I think the impact is that model used to output internal ice temp in stc (soil temp) on ice points, but now the internal ice temp is saved in its own array, I believe the stc does not contain the ice temp any more(restart non-reproducible for non-fractional grid uncoupled with tiice) . At this time, we also need to update the GFS_diagnostics.F90 to allow it output to model history files for non-fractional grid runs (e.g. LAM). The tsfc does not change for non-fractional grid, but is the composite temp for fractional grid.

On Tue, Mar 2, 2021 at 4:40 PM Dom Heinzeller notifications@github.com wrote:

I thought I modified to write tiice separately to avoid these issues. … <#m-3481226871809676753>

Yes, that is what you did and what should remain. The cleanup work here was just a follow-up, not a reversal but got misinterpreted.

@yangfanglin https://github.com/yangfanglin this is about tiice and stc, not tsfc.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/NOAA-EMC/fv3atm/issues/213#issuecomment-789239373, or unsubscribe https://github.com/notifications/unsubscribe-auth/AI7D6TP7CYGRBQUNS4572VLTBVLN5ANCNFSM4U26VHMA .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

DeniseWorthen commented 2 years ago

This is a very old issue. I will close. Please re-create the issue if required.

NOAA-EMC / fv3atm