Skip to content
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

Convert wetlands to bare ground #1878

Closed
billsacks opened this issue Oct 25, 2022 · 10 comments · Fixed by #2372
Closed

Convert wetlands to bare ground #1878

billsacks opened this issue Oct 25, 2022 · 10 comments · Fixed by #2372
Assignees
Labels
enhancement new capability or improved behavior of existing capability
Milestone

Comments

@billsacks
Copy link
Member

To remove one source of negative runoff, we are planning to use bare ground in place of wetlands. (Note that wetlands are currently just used to represent ocean points: we currently don't run with inland wetlands.)

The original proposal was to change the surface datasets to specify bare ground rather than wetlands. However, I realized that that plan would make it impossible to do the rigorous treatment of coastal areas described in #1716 . So my plan is to add a flag that allows making this change at runtime. The other advantage of this approach is that this conversion can be turned on or off with the same surface datasets.

One other small change I'll make is to remove the code in mksurfdata that forces soil properties to hard-coded default values in wetland grid cells. This forcing seems unnecessary since soil properties are already given the same default values if no data are found for a given output cell; by removing this forcing for wetland grid cells, we will be able to use any existing soil data for a grid cell where soil properties are available despite the grid cell being considered ocean by the PFT data.

@ekluzek @wwieder @slevisconsulting

@billsacks billsacks added the enhancement new capability or improved behavior of existing capability label Oct 25, 2022
@billsacks billsacks self-assigned this Oct 25, 2022
@billsacks
Copy link
Member Author

@ekluzek @wwieder @slevisconsulting - I have a few questions:

  1. Are you okay with my proposal of doing this at runtime rather than on the surface datasets?

  2. Do you agree with the idea that the default for this behavior should depend on the model version, so that CTSM52 uses the new convert-wetlands-to-bare-ground behavior but earlier versions remain unchanged? Or should we change this for all physics versions? If this will depend on model version, it looks like I'll need to add a new 5_2 phys option and compset (at a glance these don't seem to have been added yet).

  3. I thought I'd do this on the ctsm5.2.mksurfdata branch, since it will involve some minor changes to mksurfdata. Do you agree? If so, is there any standard testing being done for changes on that branch?

@billsacks billsacks added this to the ctsm5.2.0 milestone Oct 25, 2022
@billsacks billsacks added the priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations label Oct 25, 2022
@ekluzek
Copy link
Collaborator

ekluzek commented Oct 25, 2022

  1. I agree with the plan to remove this out of the datasets, to the model so that the solution is flexible. It's so much effort to create a new set of datasets for all resolutions, run time flexibility makes this much better.

  2. We had proposed that the datasets would be updated for all physics versions, so we could do this either way. We might need a clm5_2 physics option anyway, so nothing wrong with adding that. Although if ctsm5.2 is just the new datasets and we change the datasets for all versions -- maybe ctsm5.2 doesn't really mean much. So we could just do a couple ctsm5.2 tags and then make a new tag for ctsm5.3 where we start changing answers beyond surface datasets.

  3. Yes, that makes the most sense to put it on the branch
    The testing depends on what's done. And not all testing actually works, because datasets aren't updated on the branch. So we should probably chat about this to decide what to do. We have been making PR's for important updates and making sure both Sam and I know about them, and approve them. Then we make a branch tag when the PR is pulled to the branch.

@billsacks
Copy link
Member Author

Awesome, thanks @ekluzek . The code changes should be pretty small and isolated to a subroutine that will only be invoked for the new option, so I think it won't be necessary to run full testing on these changes... I think just a subset of (working) tests should be fine. But if desired, I could temporarily rebase the changes onto master in order to run full testing; we can decide once I submit a PR with the changes.

@ekluzek
Copy link
Collaborator

ekluzek commented Oct 25, 2022

Don't worry about rebasing to master for a small change. We will get all the testing working on the branch, and can solve any small problems when that happens.

@slevis-lmwg
Copy link
Contributor

I am on board with the above decisions.

@wwieder
Copy link
Contributor

wwieder commented Oct 25, 2022

My only naive question is if this would just be a compset distinction, as I'm imagining it really only matters for coupled runs?

@billsacks
Copy link
Member Author

My only naive question is if this would just be a compset distinction, as I'm imagining it really only matters for coupled runs?

I'm not sure I totally understand your question. Are you asking if we would want the behavior to differ for coupled vs uncoupled runs? We usually try to keep the behavior the same between coupled and uncoupled runs both to keep the CTSM operation more consistent in these two cases and to avoid surprises. So I was imagining that any run using CTSM52 or later – coupled or uncoupled – would use this new behavior, but you can let me know if you feel it's important for this new behavior to only be in place for coupled runs.

@wwieder
Copy link
Contributor

wwieder commented Oct 26, 2022

I guess this is true, this issue is really just arising because of a mismatch in surface data used by the land and ocean models (and points where the ocean is assigning us land that we don't know anything about, and applying wetlands to)? If we're using our own internally consistent surface data in an uncoupled run this shouldn't really be an issue?

More broadly, however, the 2nd and 3rd parts of our negative runoff fix in #1835 we only going to applied in coupled cases, but maybe this isn't accurate either?

@billsacks
Copy link
Member Author

I guess this is true, this issue is really just arising because of a mismatch in surface data used by the land and ocean models (and points where the ocean is assigning us land that we don't know anything about, and applying wetlands to)? If we're using our own internally consistent surface data in an uncoupled run this shouldn't really be an issue?

Even in an uncoupled run, we typically use the standard ocean mask so we end up with the same fractional cover. We wouldn't need to do this – and I think the standard for F compsets (AMIP-style runs) is to use a land mask that is unrelated to any ocean grid – but this is what's been done for I compsets for as long as I've been around. I think the rationale is that we want consistency between the operation of I compsets and that of fully-coupled runs. Maybe @ekluzek could provide more historical perspective. One situation to also consider is an I compset for the sake of spinning up CTSM with cplhist data (i.e., forcings from a previous B compset). It seems like we would want the land to work as similarly as possible in this case as it does in the equivalent B compset.

More broadly, however, the 2nd and 3rd parts of our negative runoff fix in #1835 we only going to applied in coupled cases, but maybe this isn't accurate either?

I haven't given this much thought, but for similar reasons, I imagined that we were going to do parts (2) and (3) for all runs, not just fully-coupled runs. But this definitely warrants discussion. Let's talk more at the ctsm-software meeting tomorrow.

@billsacks
Copy link
Member Author

I am tentatively applying the conversion of wetlands to bare ground before the collapsing to N dominant landunits or the collapsing of too-small wetland areas. This intuitively feels more correct to me and also matches the behavior we would have gotten if we did the combination on the surface dataset itself, but others can let me know if you feel we should do the collapsing of wetland areas before converting wetlands to bare ground.

billsacks added a commit to billsacks/ctsm that referenced this issue Nov 1, 2022
billsacks added a commit that referenced this issue Nov 8, 2022
Add option to convert wetlands to land; apply it by default for CLM51
physics

From #1890

This PR adds a new namelist option, "convert_ocean_to_land", which
converts all wetland area to natural veg (typically bare ground) at
runtime. This is set to true by default when using CLM51 physics.

In addition, this PR makes a minor change to the setting of soil
properties in mksurfdata: Previously, soil properties were set to some
default value outside of the pctlnd_pft-determined land mask. But I
can't see any reason why that needs to be done, and doing so could
remove some valid soil properties in the situation where there is a grid
cell that has valid soil properties even though the pctpft dataset
claims the area is ocean.

Note that this PR is into the ctsm5.2.mksurfdata branch. Most of the
changes could be applied directly to master, but there are some related
changes to mksurfdata that motivated me to make this PR into the
ctsm5.2.mksurfdata branch. However, standard f09 surface datasets don't
show any changes due to the code changes in mksurfdata here, and the
mksurfdata changes aren't critical anyway, so it would be reasonable to
rebase the non-mksurfdata changes onto master if we want them available
sooner.

Resolves #1878

Are answers expected to change (and if so in what way)? Yes: changes
answers for CLM51 cases in a few grid cells that used to be wetland but
now are classified as bare ground.

Any User Interface Changes (namelist or namelist defaults changes)? Adds
new namelist variable, "convert_ocean_to_land"

Testing performed, if any:
- Ran the following tests with comparison against the ctsm5.2mksurfdata branch:
  - `ERP_D_P36x2_Ld3.f10_f10_mg37.I1850Clm50BgcCrop.cheyenne_intel.clm-default`: passes and bit-for-bit
  - `ERP_Ld9.f45_g37.I2000Clm51Bgc.cheyenne_intel.clm-default`: passes but changes answers, as expected
  - `ERI_D_Ld9.f10_f10_mg37.I1850Clm51Bgc.cheyenne_gnu.clm-default`: passes (didn't do baseline comparisons, but expect baseline comparisons to fail)
- Also made new surface datasets with these changes in soil property mapping, 1850 f09:
  - Standard surface datasets show no differences
  - Also did a test where I temporarily set pctlnd_pft to 0 everywhere
  after the call to mkpft. I verified that this only leads to
  differences in various PCT fields: there are no longer any differences
  in soil properties arising from this change in pctlnd_pft. In
  contrast, setting pctlnd_pft to 0 before the changes in this PR leads
  to differences in many soil properties, because in this baseline
  version, soil properties were set to a constant value over grid cells
  outside the pctlnd_pft-based land mask.
@ekluzek ekluzek removed the priority: high High priority to fix/merge soon, e.g., because it is a problem in important configurations label Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new capability or improved behavior of existing capability
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

4 participants