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

make_bcs fix for bad HWSDv1.21 data in Argentina "peatland" #944

Merged
merged 13 commits into from
Jun 10, 2024

Conversation

gmao-rreichle
Copy link
Contributor

@gmao-rreichle gmao-rreichle commented May 24, 2024

In GEOS bcs versions NL5 and v11 (after the introduction of PEATMAP and PEATCLSM), a small region in Argentina (~38W,60W) is wrongly classified as peatland. The reason for the wrong classification are unrealistically high values of organic carbon content (OC) in the HWSDv1.21 input data for soil mapping unit (SMU) #12302.

make_bcs uses preprocessed soil properties data that contain merged data from HWSD and STATSGO2 for the US. The SMU in question is spread across two of the SoilProperties_*.nc files: H12V06 and H13V06. These two files were patched by replacing the clay, sand, and OC values for the 0-30cm and 30-100cm layers in the SMU in question with values obtained manually from v2 of HWSD.

The present PR enables make_bcs to use these revised SoilProperties_*.nc data to generate v12 bcs.

PR only touches make_bcs and is thus trivially 0-diff for GCM and LDAS simulations.

PR was successfully 0-diff tested for generation of existing bcs (NL3 for EASE and cube-sphere tile spaces) on 31 May 2024 by @biljanaorescanin

v12 bcs were verified by @gmao-rreichle through direct comparison of bcs files and the SMAP Nature Run v11.3 simulation.

…aram.F90, mod_process_hres_data.F90, rmTinyCatchParaMod.F90)
@gmao-rreichle gmao-rreichle added 0 diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix This fixes a bug labels May 24, 2024
@gmao-rreichle
Copy link
Contributor Author

@mathomp4 :
As a side show in the present PR, we're trying to get "make_bcs.py" to work on SLES15/Milan. My initial approach was to simply remove the #SBATCH constraint (fb06070).
But this only works on SLES12. On SLES15, the job that is submitted by make_bcs.py either hangs or crashes unexplicably with library errors. We apparently can get this to work by adding an explicit (non)-constraint
#SBATCH --constraint=sky|cas|mil (8fd5fe3). For SLES15, using just #SBATCH --constraint=mil also works.
That is, it looks like the SLES15 job won't start or run without the explicit #SBATCH constraint directive, which doesn't really make sense to me. Maybe this is something for NCCS to sort out?
cc: @biljanaorescanin @weiyuan-jiang

@mathomp4
Copy link
Member

@mathomp4 : As a side show in the present PR, we're trying to get "make_bcs.py" to work on SLES15/Milan. My initial approach was to simply remove the #SBATCH constraint (fb06070). But this only works on SLES12. On SLES15, the job that is submitted by make_bcs.py either hangs or crashes unexplicably with library errors. We apparently can get this to work by adding an explicit (non)-constraint #SBATCH --constraint=sky|cas|mil (8fd5fe3). For SLES15, using just #SBATCH --constraint=mil also works. That is, it looks like the SLES15 job won't start or run without the explicit #SBATCH constraint directive, which doesn't really make sense to me. Maybe this is something for NCCS to sort out? cc: @biljanaorescanin @weiyuan-jiang

You might ping NCCS, but now that I think about it, it's possible an sbatch with no constraint currently only targets sky|cas and not mil. I guess in gcm_setup/gcm_run we always specify the constraint, though that's because we ask the user what they want to run on (makes the math for oserver, etc. easier)

@weiyuan-jiang
Copy link
Contributor

#SBATCH --constraint=sky|cas|mil

I don't think this is correct. The executables built on SLES12 cannot be run on SLES15.

@mathomp4
Copy link
Member

#SBATCH --constraint=sky|cas|mil

I don't think this is correct. The executables built on SLES12 cannot be run on SLES15.

Oh yeah. That is true. That's the reason I have the BUILT_ON_SLES15 variable in CMake. I use it in gcm_setup and @weiyuan-jiang used it in the LDAS setup as well I think.

This is probably the "right" thing to do.

@gmao-rreichle
Copy link
Contributor Author

Thanks, @weiyuan-jiang and @mathomp4 for the commit and advice, much appreciated.
@biljanaorescanin : When you get a chance, please run just one set of bcs on both SLES12 and SLES 15 to make sure there isn't a syntax error somewhere.

@weiyuan-jiang
Copy link
Contributor

#SBATCH --constraint=sky|cas|mil

I don't think this is correct. The executables built on SLES12 cannot be run on SLES15.

Oh yeah. That is true. That's the reason I have the BUILT_ON_SLES15 variable in CMake. I use it in gcm_setup and @weiyuan-jiang used it in the LDAS setup as well I think.

This is probably the "right" thing to do.

@biljanaorescanin said the string is not replaced. Does that mean the trick does not apply to GEOSgcm_GridComp? @mathomp4

@biljanaorescanin
Copy link
Contributor

After last fix everything works now with no errors. I've run few diff sets of bcs on both sles12 and sles15

@mathomp4
Copy link
Member

@biljanaorescanin said the string is not replaced. Does that mean the trick does not apply to GEOSgcm_GridComp? @mathomp4

Ah. You figured it out before I could reply. Yeah, it's configure_file :)

@gmao-rreichle gmao-rreichle self-assigned this Jun 4, 2024
@gmao-rreichle gmao-rreichle marked this pull request as ready for review June 4, 2024 21:12
@gmao-rreichle gmao-rreichle requested review from a team as code owners June 4, 2024 21:12
@gmao-rreichle
Copy link
Contributor Author

@wmputman @sdrabenh @biljanaorescanin : This PR is now ready for merging:

  1. The PR only touches make_bcs and is trivially 0-diff for AGCM and LDAS simulations.
  2. The PR was successfully 0-diff tested for the generation of existing bcs.
  3. The new "v12" bcs have been verified in the offline (land-only) SMAP Nature Run system. In the offline system, the changes are strictly limited to the small region in Argentina where the erroneous soil parameters were replaced with more sensible values. I'm happy to share a few slides if you're interested. In the GCM, a change in one location will obviously impact the rest of the world, but it's fair to assume that the difference between the "v12" and "v11" bcs is not going to change the GCM's climate noticeably.

cc: @rdkoster

@sdrabenh sdrabenh merged commit aea9319 into develop Jun 10, 2024
9 checks passed
@sdrabenh sdrabenh deleted the bugfix/rreichle/bcs_HWSD_Argentina_peatland branch June 10, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 diff The changes in this pull request have verified to be zero-diff with the target branch. bugfix This fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants