Changes the way columns and patches are identified #7790

bishtgautam · 2025-10-13T16:54:22Z

Presently, the way a column or a patch is identified is whether they are associated with natural vegetation
or crop is to look at the landunit type.

    do c = bounds%begc,bounds%endc
       l = col_pp%landunit(c)
       
       ! Is this column a soil or a crop?
       if (lun_pp%itype(l) == istsoil .or. lun_pp%itype(l) == istcrop) then
          ! Do something on this 'c' 
       endif
      end do

    do p = bounds%begp,bounds%endp
       l = veg_pp%landunit(p)
       
       # Is the vegetation associated with soil (==natural vegetation) or a crop?
       if (lun_pp%itype(l) == istsoil .or. lun_pp%itype(l) == istcrop) then
          ! Do something for this 'p'
       endif
      end do

This PR does the following :

Adds new members to col_pp:
- is_soil
- is_crop
- is_lake
Add new members toveg_pp
- is_on_soil_col
- is_on_crop_col
Using these new code changes allows the do-loops to be such that one does not have to look at the landunit type:

    do c = bounds%begc,bounds%endc       

       ! Is this column a soil or a crop?
       if (col_pp%is_soil(c) .or. col_pp%is_crop(c)) then
          ! Do something on this 'c' 
       endif
      end do

    do p = bounds%begp,bounds%endp
       
       # Is the vegetation associated with soil (==natural vegetation) or a crop?
       if (veg_pp%is_on_soil_col(p) .or. veg_pp%is_on_crop_col(p)) then
          ! Do something for this 'p'
       endif
      end do

This change will help in the development of dynamic lake areas.

aldivi · 2025-10-21T19:41:20Z

@bishtgautam I don't see any obvious conflicts with the E3SM-GCAM code, although I am not sure if you are comparing with the current master, which includes the E3SM-GCAM code. Most of these files were not touched by adding the human component. The init and restart files appear not to change any land level weighting. As long as the new check functions the same as the old one, and all of the code controlled by it functions the same, I don't think it should be an issue.
I did find the 'lun_pp%itype(l) == istsoil' check in iac2lndMod that needs to be updated. I did not find any other of these checks related to the E3SM-GCAM code in the current master.

bishtgautam · 2025-10-21T21:22:16Z

After a few open land-related PRs are merged, I will rebase this PR off the master.

bishtgautam · 2025-11-26T03:14:33Z

Since there aren't any conflicts, rebasing isn't required. So, this is ready for review.

aldivi · 2025-12-01T17:25:53Z

@bishtgautam did you update the file in your branch mentioned above in my 21 oct comment (iac2lndMod) ? Or does this file not exist in your branch? There isn't a conflict, but this is a check that you have modified in other files:
'lun_pp%itype(l) == istsoil'
I think it needs to be updated for the code to work properly.

bishtgautam · 2025-12-01T18:42:48Z

@aldivi, Good catch. I certainly missed the fact that my branch didn't have components/elm/src/main/iac2lndMod.F90. Let me rebase this branch and fix the file you mentioned.

bishtgautam · 2025-12-01T18:47:04Z

@aldivi d685be6 now addresses your comment.

bishtgautam · 2025-12-10T16:34:22Z

@aldivi, @peterdschwartz, could you please review this? If possible, I would like to get this PR merged to master this week. Thanks.

peterdschwartz · 2025-12-10T16:44:46Z

components/elm/src/biogeochem/FanMod.F90

what happened here ?

I removed execute permission on the file.

components/elm/src/biogeochem/FanUpdateMod.F90

peterdschwartz · 2025-12-10T16:49:02Z

components/elm/src/biogeophys/CanopyFluxesMod.F90

what happened here?

I removed execute permission on the file

peterdschwartz · 2025-12-10T16:54:02Z

components/elm/src/cpl/lnd_disagg_forc.F90

another empty file?

I removed execute permission on the file

components/elm/src/main/filterMod.F90

peterdschwartz · 2025-12-10T16:58:17Z

components/elm/src/main/surfrdMod.F90

I removed execute permission on the file

peterdschwartz · 2025-12-10T16:58:24Z

components/elm/src/utils/domainMod.F90

I removed execute permission on the file

peterdschwartz · 2025-12-10T17:04:54Z

components/elm/src/biogeochem/FanUpdateMod.F90


       g = col_pp%gridcell(c)
-       if (lun_pp%itype(l) == istsoil) then
+       if (col_pp%is_soil(c)) then


i'm confused why this is needed ? we are looping over filter_soilc here where they are all soil columns?

The previous if statement for active also should be unnecessary?

we are looping over filter_soilc here where they are all soil columns?

That is a good point, and I also uncertain if we needed if (lun_pp%itype(l) == istsoil).

@bbye, Do you recall why we have that check here? Is there a test for the FAN model in the testsuite that I can test to make sure if I remove the check, the test won't be broken.

The previous if statement for active also should be unnecessary?

For the dynamic landunits, we are allocate memory at the start, but those columns might not be active at a given time step.

The filters that are used in computation routines are the filters that only include active elements though. They get recomputed in dynSubgridDriverMod prior to being used in the rest of the model. I believe the only reason to check active inside compute routines is when you are averaging up the subgrid (ie c2l) where it can be more convenient to loop over all columns

Aaah, good point. Then, I don't know the reasoning behind why the original code is checking if the column is active or not.

This was a topic of conversation in the original implementation of FAN in CTSM. The reasoning is due to how the input data is defined and how it needs to be converted. I pasted the discussion below. This might be something to address in the land data group. In the meantime, there is a fan test that is included in the land developer set you can run. I don't know how good the test will be with this issue since the test is over a single grid with only crops (SMS_Ly2_P1x1.1x1_smallvilleIA.IELMCNCROP.chrysalis_intel.elm-fan).

billsacks
on Oct 31, 2019
The col%active check is unnecessary, since the filter only includes active points. The col%wtgcell check should be removed: we try hard to avoid having code that depends on column areas, or any other subgrid areas; this can be problematic for a number of reasons that I'm happy to discuss further if you want.

billsacks
on Oct 31, 2019
I see similar checks done elsewhere in this module; those should also be removed.

juliusvira
on Dec 15, 2019
Currently some of the inputs are defined in mass per gridcell area, and they need to be converted to mass per column area. As long as this remains the case, it is not possible to avoid checking the cell weight, or to have the zero-weight columns to behave "exactly same" as others.

billsacks
on Dec 15, 2019
I think the right solution here – long-term – is to ensure that the inputs are specified in terms of mass per crop column area; then we shouldn't need these weight multipliers or checks in the code. This is important in CTSM for a variety of reasons, a big one being that FAN's input data set cannot know ahead of time how much crop area there may be in each grid cell, since that is specified independently. It's generally more important to have per-area application rates correct than it is to have total grid cell application volumes correct. Imagine, for example, that crop area doubled relative to present day. It sounds like, with the way things are set up now, that would mean that inputs would be halved on a per-area basis; this is typically not what we want.

For now, while FAN is an experimental feature, this can be left as is. But if we want FAN to become more widely supported or default, then I think we'll want to change this.

juliusvira
on Dec 15, 2019
There are two types of manure N in FAN: the N in "mixed" livestock systems, and the N in "pastoral" livestock systems. The "mixed N" is now given per crop area, so the problem doesn't occur there, but "pastoral N" is per land area. The reason is that the pastoral systems are normally not associated with crops but rather with semi-natural rangelands, often in regions that are not very suitable for growing crops. It is non-trivial to me which sub-gridcell area they should be associated with: the total native vegetation column, or just some pfts in it?

I think that scaling the manure N with land use in transient runs is somewhat questionable in any case; it would be better to have it changing with time the same way as fertilizers are. There are datasets for the historical period, and IAMs include proxies that can be used for the future. We had an AGU poster which included some initial simulations for the future scenarios.

billsacks
on Dec 17, 2019
I think this warrants an in-person / phone / zoom discussion at some point. My impression, though, is that this doesn't need to be settled now, and we can revisit it later. For now I just wanted to flag this as an issue that we'll need to revisit.

billsacks
on Feb 19, 2020
(For the record, it looks like this has not been resolved. As I note above, I'm okay with that for now.)

@bbye, Thanks for these detailed messages.

@peterdschwartz, my suggestion is to leave the if-condition as is, and we can revisit it later, because I agree with @bbye that SMS_Ly2_P1x1.1x1_smallvilleIA.IELMCNCROP.chrysalis_intel.elm-fan might not be very exhaustive.

peterdschwartz · 2025-12-10T17:05:06Z

components/elm/src/biogeochem/FanUpdateMod.F90

       l = col_pp%landunit(c)
       g = col_pp%gridcell(c)
-       if (.not. (lun_pp%itype(l) == istsoil .or. lun_pp%itype(l) == istcrop)) cycle
+       if (.not. (col_pp%is_soil(c) .or. col_pp%is_crop(c))) cycle


same as above

because of dynamic land units

peterdschwartz · 2025-12-18T18:37:06Z

There is the SMS_Ly2_P1x1.1x1_smallvilleIA.IELMCNCROP.elm-fan test. I can make the simple change and see if it causes any DIFFs. If it does, we can continue with merging. If it doesn't, i'll just push the commit to your branch

peterdschwartz · 2026-01-05T21:23:32Z

@bishtgautam Yes, I will plan to merge this tomorrow. @bbye Thanks for the detailed breakdown of past discussions.

This PR does the following : Adds new members to col_pp: is_soil is_crop is_lake Add new members toveg_pp is_on_soil_col is_on_crop_col Using these new code changes allows the do-loops to be such that one does not have to look at the landunit type: [BFB]

peterdschwartz · 2026-01-06T21:52:21Z

on next

bishtgautam marked this pull request as ready for review November 21, 2025 18:46

bishtgautam requested a review from peterdschwartz November 21, 2025 18:46

bishtgautam assigned peterdschwartz Nov 21, 2025

bishtgautam added the ELM land model label Nov 21, 2025

bishtgautam changed the title ~~[WIP] Changes the way columns and patches are identified~~ Changes the way columns and patches are identified Nov 26, 2025

bishtgautam force-pushed the bishtgautam/lnd/landunit-cleanup-v2 branch from f8dce5f to d685be6 Compare December 1, 2025 18:46

bishtgautam requested a review from aldivi December 1, 2025 18:47

bishtgautam force-pushed the bishtgautam/lnd/landunit-cleanup-v2 branch from d685be6 to b7abb03 Compare December 1, 2025 21:22

peterdschwartz reviewed Dec 10, 2025

View reviewed changes

bishtgautam added 12 commits December 31, 2025 19:35

Removes execute permission on files

cfc2175

Uses a locally defined variable for landunit index

4c6ec95

Adds spaces to make it more consitent for future refactor

5f9337a

Fixes bug

b2d35b2

Changes the check from landunit itype to column itype

b56a3c8

More changes the check from landunit itype to column itype

6fef00b

Minor fix

34ba5fe

Even more changes the check from landunit itype to column itype

1053288

Adds code to set if column is soil or crop

fb00b0f

Changes to use logicals

ce4cdb1

More code changes to use logicals

a5310ae

Changes few biophysics code changes to use logicals

b37f84c

bishtgautam added 12 commits December 31, 2025 19:36

Fixes the setting of logicals

13ce812

Changes more biophysics code changes to use logicals

843e9ab

Changes remaining code changes to use logicals

011a629

Adds code to set if veg is on a soil or crop column

a9cf428

Adds biogeochem code changes to use veg_pp logicals

ac07f76

Adds biogeophys code changes to use veg_pp logicals

9bfe76d

More code changes to use veg_pp logicals

5128c7c

Deletes changes that are not needed

e42bdbb

Changes few not conditional comparisions

9749298

Even more changes to use logicals

791e1bd

Adds new member to identify if the column is a lake

ddef90e

Uses logicals for lake columns

7de3408

bishtgautam force-pushed the bishtgautam/lnd/landunit-cleanup-v2 branch from b7abb03 to 7de3408 Compare January 1, 2026 03:36

peterdschwartz self-requested a review January 5, 2026 21:23

peterdschwartz approved these changes Jan 5, 2026

View reviewed changes

Changes the way columns and patches are identified #7790

Are you sure you want to change the base?

Changes the way columns and patches are identified #7790

Conversation

bishtgautam commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aldivi commented Oct 21, 2025

Uh oh!

bishtgautam commented Oct 21, 2025

Uh oh!

bishtgautam commented Nov 26, 2025

Uh oh!

aldivi commented Dec 1, 2025

Uh oh!

bishtgautam commented Dec 1, 2025

Uh oh!

bishtgautam commented Dec 1, 2025

Uh oh!

bishtgautam commented Dec 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterdschwartz Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterdschwartz commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peterdschwartz commented Jan 5, 2026

Uh oh!

peterdschwartz commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bishtgautam commented Oct 13, 2025 •

edited

Loading

peterdschwartz Dec 10, 2025 •

edited

Loading

peterdschwartz commented Dec 18, 2025 •

edited

Loading