-
Notifications
You must be signed in to change notification settings - Fork 319
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
Initializing itype as spval for fates patches #2935
base: master
Are you sure you want to change the base?
Conversation
combine this with #2934 ? |
@rgknox I suggest you leave this outside of #2934 for now. I'm wondering what will happen with this when the fates and aux_clm test lists are run. I'm curious if you'll see new issues when you run DEBUG level tests with FATES. If so I'm thinking it might be a bit to resolve. But, I might be wrong... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, to see a start at this so quickly. And for it to already have caught something. Things were being set and then overridden by FATES, so it won't change answers, but it's good to know that the change caught that.
I'm just requesting adding some comments to describe what's going on and why.
@@ -1051,7 +1034,24 @@ subroutine SurfaceAlbedo(bounds,nc, & | |||
fcansno(bounds%begp:bounds%endp), surfalb_inst) | |||
|
|||
else | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, so this is another place where this caught this pattern that was being done for FATES, and now you have it only done outside of FATES.
|
||
if(use_fates)then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd add a comment to this about why this is being done. Also try using nan rather than spval.
@@ -930,7 +930,7 @@ subroutine init(this, bounds_proc, flandusepftdat) | |||
c = this%f2hmap(nc)%fcolumn(s) | |||
pi = col%patchi(c)+1 | |||
pf = col%patchf(c) | |||
! patch%itype(pi:pf) = ispval | |||
patch%itype(pi:pf) = ispval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note about why this is being done, and what it will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, the comments on 925-927 explain what is going on already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me update the text a little though, its not as clear as it could be
Description of changes
On FATES patches, there should be no notion of pft associated with the patch, and therefore patch%itype should always be invalid. Even for FATES-SP, the patch should be associated with the FATES pft, which is not associated with itype. In this set of changes, itype is set to spval on fates patches. This will help prevent bugs in the future, because its use in a fates context should trigger errors, particularly in debug mode.
Specific notes
Addresses part of: #2932
Contributors other than yourself, if any:
@rosiealice @ekluzek @glemieux others
CTSM Issues Fixed (include github issue #):
Fixes: #2933
Are answers expected to change (and if so in what way)? No
Any User Interface Changes (namelist or namelist defaults changes)? No
Does this create a need to change or add documentation? Did you do so? No
Testing performed, if any:
One off tests to see if the model would run with Zeng and Wang turbulent transport. It does. Ran into machine issues testing with Meier2022, which should crash.