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

Set initial t_soisno=272 for soils and 274K for urban road #2355

Merged
merged 7 commits into from
Mar 4, 2024

Conversation

olyson
Copy link
Contributor

@olyson olyson commented Feb 8, 2024

Description of changes

Set initial t_soisno=272K for soils and 274K for urban road

Specific notes

Soil temperature was intended to be initialized at 272K, replacing the previous setting of 274K, as described in issue #1460, but was not implemented correctly. Instead urban road was set to 272K and soils were unchanged.

Contributors other than yourself, if any: @wwieder @dlawrenncar

CTSM Issues Fixed (include github issue #): #2338

Are answers expected to change (and if so in what way)? Yes. Less soil carbon at high latitudes. See here for a set of simulations evaluating this: NCAR/LMWG_dev#45

Any User Interface Changes (namelist or namelist defaults changes)? No

Testing performed, if any:
No system testing performed yet.

@olyson olyson added PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete tag: bug - impacts science next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Feb 8, 2024
@olyson olyson requested a review from ekluzek February 8, 2024 22:57
Copy link
Contributor

@wwieder wwieder left a comment

Choose a reason for hiding this comment

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

Thanks for correcting this @olyson

@ekluzek ekluzek assigned slevis-lmwg and unassigned ekluzek Feb 15, 2024
@ekluzek ekluzek removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Feb 15, 2024
@ekluzek
Copy link
Collaborator

ekluzek commented Feb 15, 2024

@slevis-lmwg will add this to a merge-tag for answer changes to come in right away.

@slevis-lmwg
Copy link
Contributor

Link to discussion of the problem.

@olyson
Copy link
Contributor Author

olyson commented Feb 20, 2024

I have added a use_fates that encompasses the code in question as suggested by @wwieder in issue #2373 . The test now passes. However, a couple of questions.

  1. The setting of t_soisno to 5deg below the freezing temperature when excess_ice is on is outside of the use_fates check and so if the test is changed so that excess_ice is on ( the current default is off), this test will fail (confirmed). I'm not sure what the future plans for excess_ice are and if it is intended for use with FATES.
  2. There is an indication from the excess_ice PR (Adding excess ground ice to CTSM  #1787) that it might be ok to set the initial soil temperature to 272K instead of tfrz-5. This would simplify the code, particularly if we want to run FATES with excess_ice on. "src/biogeophys/TemperatureType.F90 -- initial soil temperature set to 268.15 K at the cold start (might be redundant if Initialize t_soisno to 272K instead of 274K to avoid deep soil carbon in permafrost regions #1460 is added)"

@olyson
Copy link
Contributor Author

olyson commented Feb 22, 2024

Per the discussion at CTSM software meeting, my understanding is that we will not put in a use_fates statement and will simply mark the test as an expected fail. This will be revisited once #2384 is resolved. Also, @olyson will test a potential fix from @rgknox from the FATES side.

@olyson
Copy link
Contributor Author

olyson commented Feb 23, 2024

I've backed out the use_fates change and added the FATES test as an expected failure. I ran the full test suite on both derecho and izumi to make sure there weren't any other problems. All tests PASSed or FAILed as expected.

Copy link
Collaborator

@ekluzek ekluzek left a comment

Choose a reason for hiding this comment

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

Looks great.

@olyson is this ready to come to master then?

@olyson
Copy link
Contributor Author

olyson commented Feb 23, 2024

Yes, I believe it is ready, thanks.

@slevis-lmwg
Copy link
Contributor

slevis-lmwg commented Mar 1, 2024

I'm about to merge to dev170 and then start the test-suites.

@olyson if you have 5', please look over the ChangeLog that I put together for dev171. It's ok if not.

Test-suites:
cheyenne IN PROGRESS here /glade/work/slevis/git/latest_master/tests_0301-155329de
izumi DIFF (expected)

Add hillslope hydrology parameterization

Changes include multiple soil columns per vegetated landunit, additional meteorological downscaling, new subsurface lateral flow equations, and a hillslope routing parameterization.

slevis resolved conflicts:
doc/ChangeLog
doc/ChangeSum
Copy link
Contributor Author

@olyson olyson left a comment

Choose a reason for hiding this comment

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

Looks good other than see my comment about summarizing changes to answers, thanks.

doc/ChangeLog Outdated Show resolved Hide resolved
@slevis-lmwg slevis-lmwg merged commit 194635c into ESCOMP:master Mar 4, 2024
2 checks passed
@slevis-lmwg slevis-lmwg deleted the tsoisno272 branch March 4, 2024 18:37
@samsrabin samsrabin added bug something is working incorrectly science Enhancement to or bug impacting science labels Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something is working incorrectly PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete science Enhancement to or bug impacting science
Projects
Status: Done (non release/external)
Development

Successfully merging this pull request may close these issues.

5 participants