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

fates api 33 compatibility, two-stream radiation #6279

Merged
merged 14 commits into from
Apr 25, 2024

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Mar 4, 2024

This set of changes enables compatibility with FATES API 33, which brings in two-stream radiation for vegetation canopies.

@rgknox
Copy link
Contributor Author

rgknox commented Mar 4, 2024

This is based off of https://github.com/glemieux/E3SM/tree/lnd/fates-luh2 which is coming in via #5760

@rgknox
Copy link
Contributor Author

rgknox commented Mar 5, 2024

Not sure why the circleci build fails, I ran a test on perlmutter and it worked fine:
./create_test SMS_Ld20.f45_f45.IELMFATES.pm-cpu_gnu.elm-fates

I'll go ahead run the land developer test suite and report in. @glemieux and I also have to decide on new tests, if any to add in with this.

@peterdschwartz
Copy link
Contributor

@rgknox The circleCI fail is due to a circleci issue. The test that failed to build wasn't even an E3SM test case as far as I can tell. I'm guessing it's the same issue to be addressed here #6277 . So I wouldn't stress about it.

@rljacob rljacob requested a review from bishtgautam March 21, 2024 17:35
@glemieux
Copy link
Contributor

glemieux commented Mar 27, 2024

To do list (assigned to me):

  • Add two stream SMS test

@glemieux
Copy link
Contributor

Kicking off e3sm_land_developer tests.

These variables were removed with API33
@glemieux
Copy link
Contributor

glemieux commented Apr 1, 2024

Regression testing with e3sm_land_developer against master baseline results in all expected tests passing b4b. The fates testmods results in some diffs, which is expected given that the associated fates tag update includes a number of bug fixes and science updates.

@rljacob
Copy link
Member

rljacob commented Apr 12, 2024

@glemieux @peterdschwartz is this ready to merge?

@peterdschwartz
Copy link
Contributor

@rljacob If the code changes are done, then it would at least need a review from @bishtgautam

@bishtgautam
Copy link
Contributor

@glemieux @rgknox is this WIP or ready for review?

@rgknox
Copy link
Contributor Author

rgknox commented Apr 12, 2024

@rljacob , I believe it is, removing the label

Comment on lines +10 to +12
ncgen -o $FATESPARAMFILE $FATESDIR/parameter_files/fates_params_default.cdl

$FATESDIR/tools/modify_fates_paramfile.py --O --fin $FATESPARAMFILE --fout $FATESPARAMFILE --var fates_rad_model --val 2 --allpfts
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the parameter file generated on the fly by ncgen and then modified, instead of having the file added to the inutdeck server?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We update the parameter file frequently in FATES. This prevents us from having to make changes to multiple test files every time we change an irrelevant parameter (which would also be a liability to injecting bugs.. ie what if we don't update all the files correctly every time there is a change).

Note that in the future, it is intended that we will have many of these on-the-fly parameter files generated, so that we can stress test the model with many different configurations. It would not be a matter of updating 1 or two files, but think more like 5-10.

call set_fates_ctrlparms('use_lu_harvest',ival=pass_lu_harvest)
call set_fates_ctrlparms('num_lu_harvest_cats',ival=pass_num_lu_harvest_types)
call set_fates_ctrlparms('use_logging',ival=pass_logging)

if(use_fates_luh) then
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a whitespace if(use_fates_luh) then --> if (use_fates_luh) then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added white-space via 801afee

@bishtgautam bishtgautam self-requested a review April 16, 2024 02:37
@glemieux
Copy link
Contributor

glemieux commented Apr 16, 2024

@glemieux @peterdschwartz is this ready to merge?

Apologies for the delayed response. I'm currently rerunning tests after correcting an issue (via 468157c) with the fates_cold_allvars testmod in the e3sm land developer list.

@glemieux
Copy link
Contributor

glemieux commented Apr 16, 2024

Also I need to add the temporary fix for #6316.

UPDATE: done via 03371d6

peterdschwartz added a commit that referenced this pull request Apr 24, 2024
…#6279)

This set of changes enables compatibility with FATES API 33, which brings in two-stream radiation for vegetation canopies.

[non-BFB] for FATES
@peterdschwartz
Copy link
Contributor

merged to next

@peterdschwartz peterdschwartz merged commit c2c3b2a into E3SM-Project:master Apr 25, 2024
11 checks passed
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.

5 participants