CTSM add namelist option to enable multiple columns having a single pft

Description of changes

Enable ability to run patches on separate columns, i.e. to allow vegetation patches to evolve without competition from other pfts.

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)? Only when nameslist option is turned on; otherwise, no. Any User Interface Changes (namelist or namelist defaults changes)? new namelist option: use_individual_pft_soil_column Testing performed, if any:

Jan 13 '21 18:01 swensosc

I'm tentatively assigning this to @slevisconsulting . If he has time, he's going to work on this. @ekluzek I'm unassigning you, but I'll keep you as a reviewer in case you want to look at it and provide a review.

Jan 22 '21 21:01 billsacks

To first order, this looks reasonable. However, I have a number of concerns regarding both known and not-yet-known needs for this:

[no] How does this work with transient landuse?
- I know that dynpft_interp needs to be updated
- How should we determine the "template column" when a new column comes into existence? Currently, there's some code that treats the naturally vegetatated column as the template column when a new column comes into existence. The template column is used to set the initial biogeophysical state of the new column. We need some logic for how to choose the template column if there are multiple natural vegetated columns.
- I think that a growing/shrinking vegetated landunit will currently lead to an equal growth/shrinkage of all columns (so that col%wtlunit will remain unchanged); if you would want something different from that, some code changes would be needed
- Could use some testing in general
[no] For transient runs: Which columns have memory allocated for them?
- I think this will work okay as is, but it's going to have big memory use implications until we do https://github.com/ESCOMP/CTSM/issues/229
[no] Which zero-weight (on the gridcell) columns are always active? Currently, zero-weight natural veg columns are always active, in part so that they can provide a spun-up state for themselves and other columns (this ties in with the above question about determining a "template" column). But this could have serious performance implications in a transient run, where a given pft - and thus, the column it sits on - may not be needed for decades. This ties in with the next question.
[no] For transient runs: Does any more thought need to be given to how the column weights are set in a grid cell that currently has 0 natural veg, but may grow in the future? I think what happens now is that you need to specify the PFT fractions adding to 100% in all grid cells, even those that currently have no natural veg. So maybe nothing more is needed here, because these PFT fractions will now end up giving the column areas. But this now carries somewhat more importance, in that the columns with non-zero area on the landunit will be active by default.

[ ] Glaciers: handle smb forcing and maybe others

In lnd2glcMod:

  ! Make sure we haven't already assigned the coupling fields for this point
  ! (this could happen, for example, if there were multiple columns in the
  ! istsoil landunit, which we aren't prepared to handle)

See lnd2glcMod: bareland_normalization:

! Note: We currently aren't careful about how we would handle things if there are
! multiple columns within the vegetated landunit. If that possibility were introduced,
! this code - as well as the code in update_clm_s2x - may need to be reworked somewhat.

Anything else for glaciers?
- (2023-08-17) From a bit of thinking and looking through some code, I can't think of / find anything else that needs to be done for glaciers to allow multiple columns within the vegetated landunit.

[ ] Check for any assumptions that there is just one vegetated column in init_interp
- At the very least, we'll need to give each column its own index for init_interp. But there may be other changes needed as well.
- See also this todo: https://github.com/ESCOMP/CTSM/blob/0f6985da747a9e0538df63bf6e3f0ff6b4341229/src/init_interp/initInterpMindist.F90#L637-L641
[ ] Is this compatible with FATES? (If not, we should prevent running this combination)
[ ] Is this compatible with CNDV? (If not, we should prevent running this combination)
[ ] [optional] How confident are we that there aren't loops that assume only one natural vegetated column? This originally started as a vague memory of something Zack Subin told me 10 years ago, about needing to make sure we avoid loops that assume we only have one column in the natural veg landunit. But looking back at mods he sent me from 2011 (where I think he was addressing this issue for the sake of vegetated wetlands), he didn't actually have much to change, so I wonder if this concern is unfounded.

Here's an idea for a test that would give me a fair amount of confidence: Set up a case where every vegetated landunit has two columns. Do two versions of this case: one where the columns (and the pfts on those columns) appear in reverse order of the other in memory. Gridcell-level averages from these two cases should only be roundoff-level different from each other. I would want this case to have the full complement of PFTs on each column (or close to it) to ensure that there is no code that assumes that there are at most maxsoil_patches or max_patch_per_col patches on the vegetated landunit.

Before coming up with that test, here were some ideas of code searches we could do to try to find problem code:
- search for maxsoil_patches and max_patch_per_col for possible candidate issues
- could look at loops that operate over istsoil but not istcrop for possible issues (point being: anything that operates over crops would be more likely to handle this correctly... though it's still possible that those loops would assume that there is no more than maxsoil_patches patches on a landunit)
- if we trust that we left ourselves good comments, we could search for 'assum' (capturing 'assume'/'assumption'/'assuming') in the code; I have looked through relevant comments in dyn_subgrid/, but not in other directories (there are about 300 instances of 'assum' throughout the source)
- etc.
My feelings at this point on this particular todo are: if it's not too much work to set up the test idea given above (as a manual, one-time test: I don't envision this being an automated test), then that could be worthwhile to give us confidence that things are working right. If not, then it may NOT be worth the effort to look through the code for possible issues.

Jan 22 '21 22:01 billsacks

Thanks for the detailed consideration of this pull request. I think it would be useful to have a scientific discussion of some of these points, perhaps at the next Software or Science meeting. As I noted before, it does seem that one option is to only allow this for non-transient simulations if it is too complicated to figure out. We don't currently anticipate that this would be a commonly run configuration, so that should be taken into consideration. Understanding the implications for FATES seems important. Is this going to impede or help with FATES transient land use plans, for example?

On Fri, Jan 22, 2021 at 3:37 PM Bill Sacks [email protected] wrote:

To first order, this looks reasonable. However, I have a number of concerns regarding both known and not-yet-known needs for this:

How does this work with transient landuse?

I know that dynpft_interp needs to be updated

How should we determine the "template column" when a new column comes into existence? Currently, there's some code that treats the naturally vegetatated column as the template column when a new column comes into existence. The template column is used to set the initial biogeophysical state of the new column. We need some logic for how to choose the template column if there are multiple natural vegetated columns.

I think that a growing/shrinking vegetated landunit will currently lead to an equal growth/shrinkage of all columns (so that col%wtlunit will remain unchanged); if you would want something different from that, some code changes would be needed

Could use some testing in general

For transient runs: Which columns have memory allocated for them?

I think this will work okay as is, but it's going to have big memory use implications until we do #229 https://github.com/ESCOMP/CTSM/issues/229

Which zero-weight (on the gridcell) columns are always active? Currently, zero-weight natural veg columns are always active, in part so that they can provide a spun-up state for themselves and other columns (this ties in with the above question about determining a "template" column). But this could have serious performance implications in a transient run, where a given pft - and thus, the column it sits on - may not be needed for decades. This ties in with the next question.

For transient runs: Does any more thought need to be given to how the column weights are set in a grid cell that currently has 0 natural veg, but may grow in the future? I think what happens now is that you need to specify the PFT fractions adding to 100% in all grid cells, even those that currently have no natural veg. So maybe nothing more is needed here, because these PFT fractions will now end up giving the column areas. But this now carries somewhat more importance, in that the columns with non-zero area on the landunit will be active by default.

Glaciers: handle smb forcing and maybe others

In lnd2glcMod:

! Make sure we haven't already assigned the coupling fields for this point ! (this could happen, for example, if there were multiple columns in the ! istsoil landunit, which we aren't prepared to handle)

See lnd2glcMod: bareland_normalization:

! Note: We currently aren't careful about how we would handle things if there are! multiple columns within the vegetated landunit. If that possibility were introduced,! this code - as well as the code in update_clm_s2x - may need to be reworked somewhat.

Anything else for glaciers?

Check for any assumptions that there is just one vegetated column in init_interp

At the very least, we'll need to give each column its own index for init_interp. But there may be other changes needed as well.

See also this todo:

https://github.com/ESCOMP/CTSM/blob/0f6985da747a9e0538df63bf6e3f0ff6b4341229/src/init_interp/initInterpMindist.F90#L637-L641

Is this compatible with FATES? (If not, we should prevent running this combination)

Is this compatible with CNDV? (If not, we should prevent running this combination)

[optional] How confident are we that there aren't loops that assume only one natural vegetated column? This originally started as a vague memory of something Zack Subin told me 10 years ago, about needing to make sure we avoid loops that assume we only have one column in the natural veg landunit. But looking back at mods he sent me from 2011 (where I think he was addressing this issue for the sake of vegetated wetlands), he didn't actually have much to change, so I wonder if this concern is unfounded.

Here's an idea for a test that would give me a fair amount of confidence: Set up a case where every vegetated landunit has two columns. Do two versions of this case: one where the columns (and the pfts on those columns) appear in reverse order of the other in memory. Gridcell-level averages from these two cases should only be roundoff-level different from each other. I would want this case to have the full complement of PFTs on each column (or close to it) to ensure that there is no code that assumes that there are at most maxsoil_patches or max_patch_per_col patches on the vegetated landunit.

Before coming up with that test, here were some ideas of code searches we could do to try to find problem code:

search for maxsoil_patches and max_patch_per_col for possible candidate issues

could look at loops that operate over istsoil but not istcrop for possible issues (point being: anything that operates over crops would be more likely to handle this correctly... though it's still possible that those loops would assume that there is no more than maxsoil_patches patches on a landunit)

if we trust that we left ourselves good comments, we could search for 'assum' (capturing 'assume'/'assumption'/'assuming') in the code; I have looked through relevant comments in dyn_subgrid/, but not in other directories (there are about 300 instances of 'assum' throughout the source)

etc.

My feelings at this point on this particular todo are: if it's not too much work to set up the test idea given above (as a manual, one-time test: I don't envision this being an automated test), then that could be worthwhile to give us confidence that things are working right. If not, then it may NOT be worth the effort to look through the code for possible issues.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CTSM/pull/1249#issuecomment-765725667, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFABYVCJXLOQNDOUOJQQMJDS3H425ANCNFSM4WBGDCHQ .

Jan 25 '21 16:01 dlawrenncar

Thanks for the detailed consideration of this pull request. I think it would be useful to have a scientific discussion of some of these points, perhaps at the next Software or Science meeting. As I noted before, it does seem that one option is to only allow this for non-transient simulations if it is too complicated to figure out. We don't currently anticipate that this would be a commonly run configuration, so that should be taken into consideration. Understanding the implications for FATES seems important. Is this going to impede or help with FATES transient land use plans, for example? …

I just want to express interest in this PR, in case there has been some more discussions, or there are plans for that in another forum. This development would be a good starting point for some ideas I have on explicitly representing sub-grid snow heterogeneity. Also not something that would (at least in the near term) be commonly used, or something that would need transient land-use.

Feb 16 '21 12:02 kjetilaas

Thanks for that input. We will try to discuss at this week's software meeting.

On Tue, Feb 16, 2021 at 5:55 AM Kjetil Schanke Aas [email protected] wrote:

Thanks for the detailed consideration of this pull request. I think it would be useful to have a scientific discussion of some of these points, perhaps at the next Software or Science meeting. As I noted before, it does seem that one option is to only allow this for non-transient simulations if it is too complicated to figure out. We don't currently anticipate that this would be a commonly run configuration, so that should be taken into consideration. Understanding the implications for FATES seems important. Is this going to impede or help with FATES transient land use plans, for example? … <#m_2302757627976868470_>

I just want to express interest in this PR, in case there has been some more discussions, or there are plans for that in another forum. This development would be a good starting point for some ideas I have on explicitly representing sub-grid snow heterogeneity. Also not something that would (at least in the near term) be commonly used, or something that would need transient land-use.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/ESCOMP/CTSM/pull/1249#issuecomment-779818589, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFABYVDJSZLSUXDHWTY5FT3S7JTL7ANCNFSM4WBGDCHQ .

Feb 16 '21 15:02 dlawrenncar

Based on discussion at today's ctsm-software meeting, we will defer trying to combine multiple vegetated columns with transient landcover change, since that is a larger task and not high priority. I have opened #1285 to track that.

[ ] We should then add some error checking to abort the run if you try to enable this new option in a transient case. You can check get_do_transient_pfts, get_do_transient_crops and get_do_transient_lakes. I think it's harder to determine if transient glaciers are enabled, and it may be that we don't check that combination for now, and just rely on documenting that this option should not be used with evolving glaciers or any of those other options.
- Update (2023-08-17) Also get_do_transient_urban

The other things in https://github.com/ESCOMP/CTSM/pull/1249#issuecomment-765725667 probably do need to be done. It's possible that we could defer the item related to glacier surface mass balance, but I'm not sure off-hand how to detect whether we're in a run where SMB is unimportant – because it can be important to scientists even in a run without CISM at all.

Feb 18 '21 18:02 billsacks

I'm just pinging this again as it came up in #1596. It looks like there are some changes needed for this one, and @slevisconsulting is currently assigned to it. I don't know if it's active or not though. I've added "next" to it so that we'll talk about it at our next CTSM software meeting.

Jan 07 '22 20:01 ekluzek

I'm just pinging this again as it came up in #1596. It looks like there are some changes needed for this one, and @slevisconsulting is currently assigned to it. I don't know if it's active or not though. I've added "next" to it so that we'll talk about it at our next CTSM software meeting.

According to my notes from while I worked on the LILAC project: I had started on #229 (see status) and planned to look at #1249 (this PR) after.

Jan 09 '22 00:01 slevis-lmwg

It looks like this is an important development, but it looks to have a lot of work that needs to happen with it before it comes in. So I'm moving it to draft status.

@swensosc can you comment on how important this is to come in? Especially on CESM3 release timelines?

Jan 20 '24 00:01 ekluzek