ctsm5.3.039: Create driver data structure for SP mode #2952
Conversation
|
Testing Removed expected fails... 0206-092821de_gnu: 65 tests 0206-092821de_int: 167 tests 0206-092821de_nvh Does anyone (@samsrabin?) know why |
Check the baseline and see if it died as well and didn't get put into expected fails. We've been having problems with the nvhpc compiler, so personally I'm not too inclined to make sure it works if everything else is OK. But, always good to spend some time looking at it... |
There was a problem hiding this comment.
This is great, this improves the current code structure and removes some problematic logic in the code. I have a few questions on names, that we can discuss.
As a separate thing I'd like us to be thinking about what kind of design we'd like to have for FATES-SP that we can work towards. If we do some thinking there we can make sure steps like this are moving us toward what we think the end design goal might be. I think that should probably be an issue itself, where we put some design thinking in. So we should open that and reference here. This would just be thought of as a step toward that, and not doing everything desired there.
Happy to discuss and do more thinking about naming and a design goal to work towards.
But, thanks for the work here, I like that you are both fixing an immediate problem and improving the code. That's the golden thing to try to do when we can.
There is no baseline for this because it was not supposed to pass SHAREDLIB_BUILD (has an expected fail for that), but somehow it passed that stage but died in RUN |
adrifoster
changed the title
samsrabin
added
enhancement
|
@ekluzek, @rosiealice, or anyone... we have encountered some further issues. It seems that we will need some further refactoring to get this bug fixed. I spoke with @glemieux and he pointed out that Is this correct? |
|
Do you mean, not 'ever' get used, or just not get used until FATES is called at the end of the day? I guess either way this is problematic/will cause restart test failures. I guess somehow we need to pass through the SP code during initialization to fix this? |
There was a problem hiding this comment.
@adrifoster thanks for your nice work here and the cleanup of the code calling structure. I just have some thoughts on names of things, which is always a hard thing to get right. I have some suggestions and we can talk about this a bit when we meet in a little bit.
github-project-automation
bot
moved this from In progress - master/b4b-dev
to Stalled (needs review, blocked etc.)
in CTSM: Upcoming tags
ekluzek
moved this from Stalled (needs review, blocked etc.)
to In progress - master/b4b-dev
in CTSM: Upcoming tags
glemieux
moved this from Under Review
to Final Testing
in FATES Pull Request Planning and Status
samsrabin
changed the title
|
Testing results for aux_clm on derecho: 0414-091736de_gnu: 80 tests 0414-091736de_int: 192 tests 0414-091736de_nvh: 3 tests Testing results for fates on derecho: 0414-092045de_gnu: 10 tests 0414-092045de_int: 39 tests 0414-092045de_nvh: 1 test |
|
@adrifoster it looks like the differences you are seeing is a bit more wide ranging than what we expected. As we we expect lai, htop, and sai to change -- as well as whatever is dependent on them. I think we were originally thinking that it would just be those three right? It looks like the differences are in Leung dust emission related variables due to the LAI change. I could go through and make sure that's the case though. I assume that's something that we should do right? |
|
No this is what I expected. These are things related to dust and parts of
the code that used the variables calculated in canopy fluxes.
If you want to check on them that is fine with me. None of the Fates
variables changed which is good.
…On Mon, Apr 14, 2025 at 4:49 PM Erik Kluzek ***@***.***> wrote:
@adrifoster <https://github.com/adrifoster> it looks like the differences
you are seeing is a bit more wide ranging than what we expected. As we we
expect lai, htop, and sai to change -- as well as whatever is dependent on
them. I think we were originally thinking that it would just be those three
right?
It looks like the differences are in Leung dust emission related variables
due to the LAI change. I could go through and make sure that's the case
though. I assume that's something that we should do right?
—
Reply to this email directly, view it on GitHub
<#2952 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADE42IVPOXA5WSJ4N2X6E6T2ZQ3QBAVCNFSM6AAAAABWSBRO4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMBTGIZTMNJVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
*ekluzek* left a comment (ESCOMP/CTSM#2952)
<#2952 (comment)>
@adrifoster <https://github.com/adrifoster> it looks like the differences
you are seeing is a bit more wide ranging than what we expected. As we we
expect lai, htop, and sai to change -- as well as whatever is dependent on
them. I think we were originally thinking that it would just be those three
right?
It looks like the differences are in Leung dust emission related variables
due to the LAI change. I could go through and make sure that's the case
though. I assume that's something that we should do right?
—
Reply to this email directly, view it on GitHub
<#2952 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADE42IVPOXA5WSJ4N2X6E6T2ZQ3QBAVCNFSM6AAAAABWSBRO4SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDQMBTGIZTMNJVGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Awesome. Thanks for that confirmation that you were expecting this. I just did a tiny bit of checking, since, you expected it. And yes these all have to do with Leung dust emissions and the change in TLAI and TSAI would explain the difference. And the ones with the differences are only the Clm60 compsets, so would have a dependency on LAI and SAI because of Leung dust emissions. And that explains why they change. So I agree this is all good. Thanks for putting the differences in the PR, so we have the history recorded of what the differences are. |
|
Test results for test results for 19:12 $ ./cs.status.fails 0414-142152iz_nag: 47 tests |
github-project-automation
bot
moved this from Final Testing
to Ready to Integrate
in FATES Pull Request Planning and Status
github-project-automation
bot
moved this from In progress - master/b4b-dev
to Done (non release/external)
in CTSM: Upcoming tags
Description of changes
Creates a new data structure for
htop,tlai, andtsaithat is just driver data.Specific notes
Previously, fates used
htop_patch(etc.) as a driver data, but we also needed to update it and set sethtop_hist_patch. Thishtop_hist_patchwas inconsistently used throughout the CLM code, resulting in mis-matches between the PFT structures that we actually were trying to run (see #2945).Now we have the SP driver data as a separate data structure, and methods to set it. FATES sets it and passes it back to CLM through the
bc_outstruct, CLM just sets it in a 1:1 call.Contributors other than yourself, if any:
@ekluzek @rgknox @rosiealice
CTSM Issues Fixed (include github issue #): Closes #2945
Are answers expected to change (and if so in what way)? Yes - because we were incorrectly using it before, all FATES-SP compsets should change.
ALL OTHER COMPSETS SHOULD BE B4B
Any User Interface Changes (namelist or namelist defaults changes)? No
Does this create a need to change or add documentation? Did you do so? N/A
Testing performed, if any:
Will test