ESCOMP / CTSM

Community Terrestrial Systems Model (includes the Community Land Model of CESM)
http://www.cesm.ucar.edu/models/cesm2.0/land/
Other
308 stars 312 forks source link

Fix handling of bare land fluxes in lnd2glcMod #204

Open billsacks opened 6 years ago

billsacks commented 6 years ago

From @billsacks on May 8, 2017 18:37

I think the handling of bare land fluxes in lnd2glcMod: update_lnd2glc may be wrong in cases where there are landunits other than the vegetated landunit in the grid cell.

I'm specifically thinking about this code:

      ! Set vertical index and a flux normalization, based on whether the column in question is glacier or vegetated.  
      if (lun%itype(l) == istice_mec) then
         n = col_itype_to_icemec_class(col%itype(c))
         flux_normalization = 1.0_r8
      else if (lun%itype(l) == istsoil) then
         n = 0  !0-level index (bareland information)
         flux_normalization = bareland_normalization(c)
      else
         ! Other landunit types do not pass information in the lnd2glc fields.
         ! Note: for this to be acceptable, we need virtual vegetated columns in any grid
         ! cell that is made up solely of glacier plus some other special landunit (e.g.,
         ! glacier + lake) -- otherwise CISM wouldn't have any information for the non-
         ! glaciated portion of the grid cell.
         cycle
      end if

I see two possible issues:

(1) The comment in the else clause seems wrong with the current code. Specifically, the note starting with "for this to be acceptable" seems wrong now: with the flux_normalization, it seems like we're passing 0 values for bare land smb anywhere where the grid cell has 0% natural veg but < 100% glacier. Is this a problem?

(2) In the else clause, I'm thinking that we may want to abort rather than just silently cycling. At one point, I added this note in the else clause on a branch - but I never brought this to the trunk:

         ! NOTE(wjs, 2016-11-18) We abort if any such landunit is found in the do_smb
         ! filter: The code in this routine only passes SMB from the vegetated column to
         ! CISM, so if other code is handling other landunits within the do_smb filter,
         ! there will likely be conservation problems due to this inconsistency.
         ! (Although I haven't checked carefully to see if there would truly be
         ! conservation problems.)

and then I had changed the cycle to:

         write(iulog,*) subname//' ERROR: column that is neither ice_mec nor soil'
         write(iulog,*) 'encountered in the do_smc_c filter.'
         call endrun(decomp_index=c, clmlevel=namec, msg=errMsg(sourcefile, __LINE__))

I want to think a little more about what the right thing is to do here.

cc @whlipscomb

Copied from original issue: NCAR/CLM#6

billsacks commented 6 years ago

The bare land fluxes in lnd2glcMod also need to be fixed to handle multiple vegetated columns, e.g., for the hillslope hydrology being developed by @swensosc and others. For example, do we need to take the average value across all of the vegetated columns in the grid cell? We'll need to think through both what makes sense scientifically and what is needed for conservation.

billsacks commented 2 years ago

Making this high priority to get it working with multiple vegetated columns (https://github.com/ESCOMP/CTSM/issues/204#issuecomment-396708549). While I'm doing that, I should probably also think about the original comment that started this issue.

billsacks commented 2 years ago

On the hillslope hydrology branch that will soon come to master (#1715 ) I have put in place a temporary workaround that will let hillslope hydrology runs pass. We should back out this change – the addition of .and. .not. use_hillslope to an error check conditional – when we resolve this issue.

See 99cfb53f2.

wwieder commented 1 year ago

@billsacks when you're available, some advice on this would be helpful, especially as #1715 is nearly ready to be merged.

billsacks commented 1 year ago

Yes, I can help with this – either doing it myself (probably a few days of work) or advising someone else. There's a release crunch for ESMF right now, but my time should open up more in a couple of weeks. Hillslope can come in without this being resolved: there is a temporary workaround in place for now.

mvdebolskiy commented 10 months ago

Quick question: which landunits are considered bareground for CISM? only vegetated or crops too?

billsacks commented 10 months ago

Good question @mvdebolskiy . For the purpose of this piece of code relevant to this issue, we just consider natural vegetation. But for other purposes, CISM divides the world into glacier/ice sheet vs. everything else, so from CISM's land cover perspective, anything that isn't glacier / ice sheet is considered "bare ground" in some sense.

billsacks commented 5 months ago

@wwieder @ekluzek - I'm thinking this can wait until after the June 30 science functionality freeze since I think the biggest issue is the handling of multiple vegetated columns on a grid cell, and I'm thinking that's still not going to be the case for any scientifically-supported configurations. But let me know if you feel differently.

wwieder commented 5 months ago

Yes, I think this can happen after a summer code freeze. Thanks Bill.