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.

Sign up for GitHub

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

Jump to bottom

Soil gas diffusivity bugfix #2157

Merged
merged 16 commits into from
Nov 4, 2023
Merged

Soil gas diffusivity bugfix #2157

merged 16 commits into from
Nov 4, 2023

Conversation

samsrabin
Copy link
Collaborator

@samsrabin samsrabin commented Sep 14, 2023 •
edited
Loading

Description of changes

Resolves two bugs in the calculation of diffusion in SoilBiogeochemNitrifDenitrif(). Also does some rearranging and renaming to improve clarity.

Specific notes

From #1990:

Contributors other than yourself, if any: @jinmuluo, @wwieder

CTSM Issues Fixed:

Are answers expected to change (and if so in what way)? Yes. Of outputs in aux_clm tests, only F_N2O_DENIT is affected.

Any User Interface Changes (namelist or namelist defaults changes)? No.

Testing performed:

aux_clm:

  1. 7d2979c (several commits comprising bug fixes and comments) shows diffs from parent ctsm5.1.dev133 only in history field F_N2O_DENIT. (At least on Cheyenne; my Izumi test has been wiped. Can rerun if needed.)
  2. 728b7e6 adds some parentheses to simulate upcoming order-of-operations changes. Diffs from 7d2979c, but again (for Cheyenne), only in F_N2O_DENIT.
  3. Commits through 3695c60 do some rearranging and renaming. Bit-for-bit identical to 728b7e6.

@samsrabin
Corrected a term in soil gas diffusivity equation.
3dfa6aa
@samsrabin
Added comments referring to variables in Riley et al. (2011).
07e1a9a
@samsrabin
Typo fix in a comment.
819f985
@samsrabin
Correct bug in calculation of ratio_k1 in SoilBiogeochemNitrifDenitri…
8e65ee1 
…f().
@samsrabin
Added caveat about diffus output where that field is set up.
65506be
@samsrabin
Added a clarifying comment in master_list_nofates.rst.
c126e2e
@samsrabin
Merge tag 'ctsm5.1.dev133' into soil-gas-diffusivity-bugfix
7d2979c 
FATES API update to facilitate fates refactor

This updates a number of FATES type names and module use statements
which correspond with a refactoring effort that moves FATES
patches and cohorts into their own respective modules.

With the FATES update is a minor science update, so there are
changes to answers for FATES.

This also incorporates a minor update to a more recent version
of the ccs config external.

# Conflicts:
#	doc/source/users_guide/setting-up-and-running-a-case/master_list_nofates.rst
@samsrabin
Adding some parentheses to simulate potential OOO changes from upcomi…
728b7e6 
…ng change.
@samsrabin
Clarifying calculation of diffus in SoilBiogeochemNitrifDenitrif().
d0a2329
@samsrabin
Renamed eps to fc_air_frac.
40dab0e
@samsrabin
Renamed f_a to fc_air_frac_as_frac_porosity.
056ec56
@samsrabin
Reverted rearrangement of fc_air_frac_as_frac_porosity calculation.
3695c60
@samsrabin
Merge branch 'soil-gas-diffusivity-bugfix-dev' into soil-gas-diffusiv…
7ae1372 
…ity-bugfix
@samsrabin samsrabin added bug something is working incorrectly tag: bug - impacts science next this should get some attention in the next week or two. Normally each Thursday SE meeting. labels Sep 14, 2023
@samsrabin samsrabin self-assigned this Sep 14, 2023
@samsrabin
Copy link
Collaborator Author

@jinmuluo Would you mind having a look at the changes here to make sure they all look good?

wwieder
wwieder reviewed Sep 14, 2023
View reviewed changes
Copy link
Contributor

@wwieder wwieder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you run tests to evaluate the impact of this bug fix on fluxes simulated in the model?

src/soilbiogeochem/SoilBiogeochemNitrifDenitrifMod.F90 Outdated Show resolved Hide resolved
src/soilbiogeochem/SoilBiogeochemNitrifDenitrifMod.F90 Show resolved Hide resolved
@samsrabin
Copy link
Collaborator Author

@wwieder If you're asking about scientific tests of the impact, no, I haven't. I do have all the (Cheyenne) aux_clm outputs, though, so we could look at those.

@wwieder
Copy link
Contributor

wwieder commented Sep 14, 2023

I think it's worth running a longer case to understand the scientific impacts on the bug fix, or is the answer changing part of this just due to roundoff changes in the order of operations?

@samsrabin
Copy link
Collaborator Author

A longer test makes sense, @wwieder, since the diffs aren't just roundoff-level. How long do you think, and at what resolution?

@wwieder
Copy link
Contributor

wwieder commented Sep 19, 2023

My default is for a full spinup from cold start and then historical simulation at 1 degree, but lately we've been testing things out more with the sparse grid from the PPE. What do you think @slevis-lmwg, @dlawrenncar and @linniahawkins?

@slevis-lmwg
Copy link
Contributor

@wwieder I don't have first-hand experience with the PPE sparse grid; however, it seems like a reasonable time-saving approach to use it.

@linniahawkins
Copy link

Probably depends on the application - for global scales and primary variables the sparse grid is quite accurate but issues may arise at regional scales or for specific PFTs. A practical issue might be that it doesn't play nice with NCL, Daniel has some nice python code to project the sparse grid back to full grid, but it adds an extra step. Here's a visual of where the 400 cells are located (blue points):
Sparsegrid_cell_locations

@samsrabin
Copy link
Collaborator Author

Thanks, @linniahawkins! @wwieder, looks good to me; what do you think?

@wwieder
Copy link
Contributor

wwieder commented Sep 27, 2023

Sorry, what are you asking here, @samsrabin? If it's OK to evaluate effects of the bug fix on ecosystem states and fluxes using the sparse grid instead of a full global simulation? I think this seems like a reasonable way to quantify the magnitude of the bug fix. Or at least to try... My question more on the process side of things, (e.g. how to we understand / quantify answer changing tags & can we leverage the sparse grid)? I don't think we can use the PPE branch tag here, as it's not up to date with main, but you still should be able to use the sparse grid surface dataset.

At some point I'd be curious to have a closer look at differences between Daniel's extrapolation from the 400 points back to the globe and a gridded simulation, but that's not really the question here.

@samsrabin
Copy link
Collaborator Author

Marking as blocked because I want to run with the PPE sparse grid, but sparse grids are incompatible with NUOPC.

@samsrabin samsrabin added the blocked: dependency Wait to work on this until dependency is resolved label Oct 4, 2023
@dlawrenncar
Copy link
Contributor

dlawrenncar commented Oct 4, 2023 via email

@samsrabin
Copy link
Collaborator Author

@dlawrenncar Just for now. @adrifoster, @slevis-lmwg, and @ekluzek are working on adding the capability.

@ekluzek
Copy link
Collaborator

ekluzek commented Oct 4, 2023

@dlawrenncar see #1731 to see where we are at in the process. I did get a test case working with an earlier version of this. But, we didn't bring it in as something that we test. And since that time mesh_modifier was completed and will be the way we make the process replicatable.

Once, @adrifoster has this working, we will save it as a user-mod directory and start testing with it. And we will update the notes on the process to make sure we have a process that can be replicated. @adrifoster also found a file in the PPE work that we can base these files off of.

@slevis-lmwg
Copy link
Contributor

@dlawrenncar Just for now. @adrifoster, @slevis-lmwg, and @ekluzek are working on adding the capability.

Generally speaking, it works, e.g. Xiulin's work with Charlie and Lara.
The workflow involves a somewhat long list of steps.
We (@adrifoster @ekluzek @slevis-lmwg) are working on implementing on the PPE sparse grid now.

@samsrabin samsrabin removed the next this should get some attention in the next week or two. Normally each Thursday SE meeting. label Oct 5, 2023
@samsrabin
Merge tag 'ctsm5.1.dev142' into soil-gas-diffusivity-bugfix
d369f8d 
b4b changes to Python scripts, history lists, tech note, and clm_time_manager.

* Add system and unit tests for making fsurdat with all crops everywhere (ESCOMP#2081)
* Rework master_list* files etc. (ESCOMP#2087)
* Fixes to methane Tech Note (ESCOMP#2091)
* Add is_doy_in_interval() function (ESCOMP#2158)
* Avoid using subprocess.run() in FSURDATMODIFYCTSM (ESCOMP#2125)

Closes issues:
* Add unit test for making fsurdat with all crops everywhere (ESCOMP#2079)
* Rework master_list_(no)?fates.rst? (ESCOMP#2083)
* conda run -n can fail if a conda environment is already active (ESCOMP#2109)
* conda fails to load for SystemTests (ESCOMP#2111)
@samsrabin samsrabin removed the blocked: dependency Wait to work on this until dependency is resolved label Oct 9, 2023
@samsrabin
Copy link
Collaborator Author

I'll go ahead and test with a 4x5 grid instead of waiting for the NUOPC sparse grid.

@samsrabin
Copy link
Collaborator Author

It looks like this results in a reduction of mean global diffusivity by about 12.7%:
screenshot_6517

This ranges from about 0–35% across gridcells in two years I looked at:
screenshot_6518

@wwieder Because of how quickly diffusivity levels out, it doesn't seem necessary to do another run but from a spinup with the fix applied. What do you think?

@wwieder
Copy link
Contributor

wwieder commented Oct 26, 2023 via email

@samsrabin
Copy link
Collaborator Author

samsrabin commented Oct 26, 2023 •
edited
Loading

The only variable that had any diffs in (Cheyenne tests of) aux_clm was F_N2O_DENIT. This has larger relative differences, with the new code resulting in a greater flux, especially in the boreal zone:
screenshot_6519

screenshot_6520

@wwieder
Copy link
Contributor

wwieder commented Oct 26, 2023

Great. I think this is good to merge. Thanks, Sam.

@jinmuluo
Copy link

jinmuluo commented Oct 27, 2023 via email

@samsrabin samsrabin added the PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete label Oct 27, 2023
@samsrabin samsrabin merged commit 9b70f5a into ESCOMP:master Nov 4, 2023
2 checks passed
@samsrabin samsrabin added the science Enhancement to or bug impacting science label Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wwieder wwieder wwieder left review comments

Assignees

@samsrabin samsrabin

Labels
bug something is working incorrectly PR status: ready PR: this is ready to merge in, with all tests satisfactory and reviews complete science Enhancement to or bug impacting science
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Problems about the soil gas diffusivity in methane code and nitrification-denitrification mod
7 participants
@samsrabin @wwieder @slevis-lmwg @linniahawkins @dlawrenncar @ekluzek @jinmuluo