Skip to content

fix missing connection of ICE and FRZR in some of the SURF components#1111

Merged
sdrabenh merged 12 commits intodevelopfrom
bugfix/zhaobin74/fix-missing-connection-ice-frzr
Jun 27, 2025
Merged

fix missing connection of ICE and FRZR in some of the SURF components#1111
sdrabenh merged 12 commits intodevelopfrom
bugfix/zhaobin74/fix-missing-connection-ice-frzr

Conversation

@zhaobin74
Copy link
Contributor

@zhaobin74 zhaobin74 commented May 30, 2025

This PR partially fixed #1110. There will be additional independent efforts coming

NOTE: FRZR is combined with SNO and ICE to form total frozen precip, as done in catchment to keep consistency. FRZR could be part of liquid precip based on discussion. Should that is the case, another PR is needed to change across all surf components. Right now, FRZR is zero so it really does not matter.

Based on the consensus, FRZR should be treated as liquid, so it is combined with PLS and PCU to form RAIN.

The PR is 0-diff for AMIP and non 0-diff for coupled configurations.

Due to scope change, this PR is now non 0-diff.


This PR partly fixes #1110.

This PR is 0-diff for AMIP and non 0-diff for coupled configurations. The fixes are implemented in Saltwater and Catchment. For the latter, FRZR was removed from total solid precip.

A separate Non-0-diff PR will address Lake and Landice component.

@zhaobin74 zhaobin74 requested review from a team as code owners May 30, 2025 15:34
@zhaobin74 zhaobin74 added 0-diff AMIP 0-diff for uncoupled AMIP runs Not 0-diff coupled Non 0-diff for coupled AGCM/MOM5/MOM6/CICE4/CICE6 model labels May 30, 2025
@zhaobin74
Copy link
Contributor Author

zhaobin74 commented May 30, 2025

This PR is in 0-diff uncoupled category but I don't see it in the list. I can change it if someone could point me to the correct label. Thanks.

@zhaobin74 zhaobin74 marked this pull request as draft May 30, 2025 17:12
@zhaobin74 zhaobin74 marked this pull request as ready for review June 4, 2025 14:29
@zhaobin74 zhaobin74 requested review from a team as code owners June 4, 2025 14:29
lcandre2
lcandre2 previously approved these changes Jun 4, 2025
@gmao-rreichle
Copy link
Contributor

@zhaobin74 : It's probably best to have just one PR for all of the FRZR changes. Then it's easier to make sure everything is consistent across components. Indeed, Lake also needs updating... We can add the Catchment changes to the branch.

@zhaobin74
Copy link
Contributor Author

@zhaobin74 : It's probably best to have just one PR for all of the FRZR changes. Then it's easier to make sure everything is consistent across components. Indeed, Lake also needs updating... We can add the Catchment changes to the branch.

Sounds good, @gmao-rreichle. I'll update the title to reflect the scope change.

@zhaobin74 zhaobin74 changed the title fix missing connection of ICE and FRZR in subcomponents of Saltwater fix missing connection of ICE and FRZR in some of the SURF components Jun 4, 2025
@zhaobin74 zhaobin74 marked this pull request as draft June 4, 2025 17:38
@zhaobin74 zhaobin74 added Non 0-diff The changes in this pull request are non-zero-diff and removed 0-diff AMIP 0-diff for uncoupled AMIP runs Not 0-diff coupled Non 0-diff for coupled AGCM/MOM5/MOM6/CICE4/CICE6 model labels Jun 4, 2025
@gmao-rreichle
Copy link
Contributor

@zhaobin74 : It's probably best to have just one PR for all of the FRZR changes. Then it's easier to make sure everything is consistent across components. Indeed, Lake also needs updating... We can add the Catchment changes to the branch.

Sounds good, @gmao-rreichle. I'll update the title to reflect the scope change.

@zhaobin74 , @lcandre2 : Apologies, I didn't realize that the planned Landice changes are non-0-diff. My assumption was that we only reallocate FRZR, which is identical to 0 in the current and forthcoming model versions (v11, v12). This is to make sure FRZR is in the right place if it is ever filled with something. Now I understand that Landice was a bit further behind and had an error in the old precipitation variables. Given that, I'm wondering if it's not better, after all, to separate the changes into two PRs, non-0-diff for Landice and 0-diff for all other components (assuming Lake is also 0-diff). In any case, I will add the Catchment changes to this branch. I'll defer to you about making the non-0-diff Landice changes here or on a separate PR. Apologies for the misunderstanding and confusion.

@zhaobin74
Copy link
Contributor Author

zhaobin74 commented Jun 5, 2025

@zhaobin74 : It's probably best to have just one PR for all of the FRZR changes. Then it's easier to make sure everything is consistent across components. Indeed, Lake also needs updating... We can add the Catchment changes to the branch.

Sounds good, @gmao-rreichle. I'll update the title to reflect the scope change.

@zhaobin74 , @lcandre2 : Apologies, I didn't realize that the planned Landice changes are non-0-diff. My assumption was that we only reallocate FRZR, which is identical to 0 in the current and forthcoming model versions (v11, v12). This is to make sure FRZR is in the right place if it is ever filled with something. Now I understand that Landice was a bit further behind and had an error in the old precipitation variables. Given that, I'm wondering if it's not better, after all, to separate the changes into two PRs, non-0-diff for Landice and 0-diff for all other components (assuming Lake is also 0-diff). In any case, I will add the Catchment changes to this branch. I'll defer to you about making the non-0-diff Landice changes here or on a separate PR. Apologies for the misunderstanding and confusion.

@gmao-rreichle, no problem. You are welcome to add catchment update here. @lcandre2, I am ok with either a non-0-diff PR specific to LANDICE or adding it in this PR.

@gmao-rreichle
Copy link
Contributor

@zhaobin74, @lcandre2 : I added the changes to Catchment and the precipitation corrections in Surface in this commit: 0ef0caa
Besides adding comments, the changes consist of two key edits:

  • Remove FRZR in the calculation of solid precipitation.
  • Remove FRZR in the calculation of total precipitation when the sum includes large-scale and convective rainfall (PLS and PCU). After talking with @wmputman, my understanding is that FRZR (if it were nonzero) would be included in the sum PLS+PCU. I hope I got this right.

Going forward, Bill said we may want to replace the PLS and PCU inputs with a new export from Moist GC called "RAIN", which does not include FRZR. However, changing from PLS and PCU to RAIN and FRZR would be a bigger undertaking. Note also that PCU and PLS are part of the standard "land forcing" (lfo) file Collection, which provide surface met forcing data for offline (GEOSldas) simulations. I don't have the bandwidth to address this at the moment.

It looks like Lake GC also uses precipitation, so it also needs fixing. Not sure who's in charge of Lake GC.

Please let me know if the above doesn't sound right. Thanks!

cc: @biljanaorescanin

@zhaobin74
Copy link
Contributor Author

zhaobin74 commented Jun 9, 2025

@zhaobin74, @lcandre2 : I added the changes to Catchment and the precipitation corrections in Surface in this commit: 0ef0caa Besides adding comments, the changes consist of two key edits:

  • Remove FRZR in the calculation of solid precipitation.
  • Remove FRZR in the calculation of total precipitation when the sum includes large-scale and convective rainfall (PLS and PCU). After talking with @wmputman, my understanding is that FRZR (if it were nonzero) would be included in the sum PLS+PCU. I hope I got this right.

@gmao-rreichle, sorry, I am a little bit confused now about FRZR. I get it that FRZR is part of liquid precip. So could you and @wmputman confirm the following?

in current code: total liquid precip = PLS + PCU because FRZR(0 or not) is already included in PCU+PLS

in future: total liquid precip = RAIN + FRZR with RAIN an export from MOIST which does not include freezing rain.

Thanks,

Going forward, Bill said we may want to replace the PLS and PCU inputs with a new export from Moist GC called "RAIN", which does not include FRZR. However, changing from PLS and PCU to RAIN and FRZR would be a bigger undertaking. Note also that PCU and PLS are part of the standard "land forcing" (lfo) file Collection, which provide surface met forcing data for offline (GEOSldas) simulations. I don't have the bandwidth to address this at the moment.

It looks like Lake GC also uses precipitation, so it also needs fixing. Not sure who's in charge of Lake GC.

@lcandre2 and I will work on Lake component.

Please let me know if the above doesn't sound right. Thanks!

cc: @biljanaorescanin

@zhaobin74
Copy link
Contributor Author

Thank you, @lcandre2, for fixing the Lake GC. This PR is ready for review.

@zhaobin74 zhaobin74 marked this pull request as ready for review June 17, 2025 15:39
@lcandre2
Copy link

Actually, I just pushed two small changes. Now it is ready.

@gmao-rreichle
Copy link
Contributor

@zhaobin74 @lcandre2 : Is this PR still 0-diff for AMIP (as indicated by the label) after the Lake changes? Wouldn't the (previously missing) icefall change the surface temp at least a little bit?

@lcandre2
Copy link

@zhaobin74 @lcandre2 : Is this PR still 0-diff for AMIP (as indicated by the label) after the Lake changes? Wouldn't the (previously missing) icefall change the surface temp at least a little bit?

Great question and I was pondering tha myself last night. My guess is it no longer zero diff, but I haven't run the tests -- happy to, but the exact tests weren't documented on the wiki.

@gmao-rreichle
Copy link
Contributor

@zhaobin74 @lcandre2 : Is this PR still 0-diff for AMIP (as indicated by the label) after the Lake changes? Wouldn't the (previously missing) icefall change the surface temp at least a little bit?

Great question and I was pondering tha myself last night. My guess is it no longer zero diff, but I haven't run the tests -- happy to, but the exact tests weren't documented on the wiki.

FYI, tests are failing right now because I updated the branch after #1112 was merged into develop (reflexively, without thinking...). Now we need to wait for GEOSgcm to catch up (GEOS-ESM/GEOSgcm#938). Sorry for the confusion

@zhaobin74
Copy link
Contributor Author

@zhaobin74 @lcandre2 : Is this PR still 0-diff for AMIP (as indicated by the label) after the Lake changes? Wouldn't the (previously missing) icefall change the surface temp at least a little bit?

Great question and I was pondering tha myself last night. My guess is it no longer zero diff, but I haven't run the tests -- happy to, but the exact tests weren't documented on the wiki.

@lcandre2 and @gmao-rreichle, you are both right. With the Lake change, it is no longer 0-diff. Maybe we can just bring all together into this PR. I'll update the label. Sorry, I missed that.

@zhaobin74 zhaobin74 added Non 0-diff The changes in this pull request are non-zero-diff and removed 0-diff AMIP 0-diff for uncoupled AMIP runs Not 0-diff coupled Non 0-diff for coupled AGCM/MOM5/MOM6/CICE4/CICE6 model labels Jun 18, 2025
@biljanaorescanin biljanaorescanin marked this pull request as draft June 24, 2025 13:08
…changes that come with the revisions of GEOS_LakeGridComp.F90)
@zhaobin74 zhaobin74 added 0-diff AMIP 0-diff for uncoupled AMIP runs Not 0-diff coupled Non 0-diff for coupled AGCM/MOM5/MOM6/CICE4/CICE6 model and removed Non 0-diff The changes in this pull request are non-zero-diff labels Jun 25, 2025
@github-actions
Copy link

This PR is being prevented from merging because you have not added one of our required labels: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled, github_actions. Please add one so that the PR can be merged.

@zhaobin74 zhaobin74 marked this pull request as ready for review June 25, 2025 21:36
@sdrabenh sdrabenh added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Jun 27, 2025
@sdrabenh sdrabenh merged commit a6a6f9b into develop Jun 27, 2025
12 of 14 checks passed
@sdrabenh sdrabenh deleted the bugfix/zhaobin74/fix-missing-connection-ice-frzr branch June 27, 2025 19:29
@sdrabenh
Copy link
Collaborator

@lcandre2 this PR has been merged into develop. Please make a new PR into develop once you made the companion lake and landice changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0-diff AMIP 0-diff for uncoupled AMIP runs 0 diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix This fixes a bug Not 0-diff coupled Non 0-diff for coupled AGCM/MOM5/MOM6/CICE4/CICE6 model

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing connectivity of ICE and FRZR for all SURF's child components except LAND

5 participants