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

Drydeposition #88

Draft
wants to merge 12 commits into
base: noresm
Choose a base branch
from
Draft

Drydeposition #88

wants to merge 12 commits into from

Conversation

rosiealice
Copy link
Collaborator

Description of changes

Does not update the science parts of the season_index calculation, yet..
To pair with forthcoming FATES-side changes.

Specific notes

Contributors other than yourself, if any:

CTSM Issues Fixed (include github issue #):

Are answers expected to change (and if so in what way)?

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

Does this create a need to change or add documentation? Did you do so?

Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on derecho for intel/gnu and izumi for intel/gnu/nag/nvhpc is the standard for tags on master)

NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).

@mvertens mvertens self-requested a review October 10, 2024 09:28
@mvertens
Copy link

@rosiealice - thanks for this. Can you summarize what test(s) you ran with this - and what code base? That would be super helpful. Actually - looking at the PR changes - I'm not sure this is needed for this PR - but its good practice to include this.
@mvdebolskiy - I have reviewed these changes and approved. Do you want to want to review as well - or should I just merge?

src/biogeochem/DryDepVelocity.F90 Outdated Show resolved Hide resolved
src/biogeochem/DryDepVelocity.F90 Outdated Show resolved Hide resolved
src/biogeophys/CanopyStateType.F90 Outdated Show resolved Hide resolved
@@ -1608,7 +1610,8 @@ subroutine wrap_update_hlmfates_dyn(this, nc, bounds_clump, &
z0m(col%patchi(c)+1:col%patchf(c)) = 0.0_r8
displa(col%patchi(c)+1:col%patchf(c)) = 0.0_r8
dleaf_patch(col%patchi(c)+1:col%patchf(c)) = 0.0_r8

wesley_pft_index_patch(col%patchi(c)+1:col%patchf(c)) = 0
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you do 0.0_r8 here, like it is done everywhere?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In DryDepVelocity the initital valaue is -1 and there are checks <0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are defined at integers in Canopystatemod so I think they should just be '0'?

Would it be good form to start this as the unset values of -1? Or just stick to 'zaeroing'?

@mvdebolskiy
Copy link
Collaborator

One of my issues is that with use_fates conditions, which includesuse_fates_sp, there will be different behaviour for SP mode than it was before.

@mvdebolskiy
Copy link
Collaborator

Also, Can you add checks for valid index values either in DryDepVelocity or in clmfates_interfaceMod where values from fates are passed? There should be an endrun with Wesley ... index out of bounds: <values> <valid_range>.

@rosiealice
Copy link
Collaborator Author

Hi @mvertens. Maybe hold off on merging it yet. I am still testing the FATES side changes, so this is not yet finished...

@mvertens
Copy link

@rosiealice - would be good to have this in draft form if you are still testing. And I've been talking to @mvdebolskiy and we feel that we should have a pared down test suite for betzy on our end that is always run as part of PRs. Let's talk more at tomorrow's meeting.

@rosiealice rosiealice marked this pull request as draft October 10, 2024 11:02
@rosiealice
Copy link
Collaborator Author

Sure. I pressed the 'convert to draft' button. Was that right? :)

@mvertens
Copy link

Perfect!

@rosiealice
Copy link
Collaborator Author

@mvdebolskiy I changed the logic here
https://github.com/rosiealice/ctsm/blob/2e4293d1281dbac04684488e75b4c188c8387df0/src/main/clm_initializeMod.F90#L707
so that the .not.use_fates statement is a different if statement to the use_fates_sp. Does that fix the concern you voiced earlier?

if ( n_drydep > 0 .and. .not. use_fates ) then
call readAnnualVegetation(bounds_proc, canopystate_inst)
! Call interpMonthlyVeg for dry-deposition so that mlaidiff will be calculated
! This needs to be done even if FATES, CN or CNDV is on!
call interpMonthlyVeg(bounds_proc, canopystate_inst)
endif
if ( use_fates_sp ) then
! If fates has satellite phenology enabled, get the monthly veg values
! prior to the first call to SatellitePhenology()
call interpMonthlyVeg(bounds_proc, canopystate_inst)
end if

@rosiealice
Copy link
Collaborator Author

I made paired CLM-SP and FATES-SP simulations to compare the dry deposition outputs.

The CLM-SP simulation is here:
/cluster/work/users/rosief/git/NorESM_alpha07test/cime/scripts/CLM6-SP.n1850.ne30_tn14.drydep

and the FATES-SP case is here:
(I have notes on how I created these, jut need to think of a place to put them)
/cluster/work/users/rosief/git/NorESM_drydep_test/cime/scripts/FATES-SP.n1850.ne30_tn14.drydep

I ran these for three years each with -drydep specified by
./xmlchange CLM_BLDNML_OPTS="-drydep" --append

This is a random month of output run through my crude plotting script (working on getting these into a diagnostics package is on our to do list).

The differences are very small (suspiciously so??!) but nonetheless, here they are.

As for the MEGAN PR, I think this code is ready for running through tests. @mvdebolskiy @mvertens

Plotted by gridcell

drydep_DSTDEP
drydep_BCDEP
drydep_OCDEP

Plotted by latitude

drydep_lat_DSTDEP
drydep_lat_BCDEP
drydep_lat_OCDEP

Only the differences plotted (to check they are not all zeros)

drydep_diffs_OCDEP
drydep_diffs_DSTDEP
drydep_diffs_BCDEP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants