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

refactor of land NP supplementation flags & fates fixation updates #5604

Merged
merged 29 commits into from
May 16, 2023

Conversation

rgknox
Copy link
Contributor

@rgknox rgknox commented Apr 18, 2023

This set of changes refactors the logic behind N and P supplementation in land bgc.
Also, ecosystem (free-living) N fixation was enabled for fates runs.

[non-BFB] for FATES tests

rgknox and others added 29 commits March 27, 2023 10:13
…n up or down (current setting is 1.0, unchanged)
…ged during updates to fates-elm nutrient coupling
@rgknox
Copy link
Contributor Author

rgknox commented Apr 18, 2023

land developer tests running on perlmutter: /pscratch/sd/r/rgknox/e3sm_tests/E03ee01df93-Fb8b905cc-spinup-v3

A previous version passed, but I had not merged in minor update, so I'm re-testing.

@rgknox
Copy link
Contributor Author

rgknox commented Apr 18, 2023

cc: @evasinha @dmricciuto @bbye

@rgknox
Copy link
Contributor Author

rgknox commented Apr 19, 2023

Testing generates differences from BASELINE in FATES test, this is expected. It also generates the following computation time increases:

TPUTCOMP Error: TPUTCOMP: Computation time increase > 10% from baseline

SMS_Ly2_P1x1.1x1_smallvilleIA.IELMCNCROP.pm-cpu_gnu.elm-force_netcdf_pio
SMS_Ly2_P1x1.1x1_smallvilleIA.IELMCNCROP.pm-cpu_gnu.elm-lulcc_sville

Are these also expected or typical?

@amametjanov
Copy link
Member

@ndkeen can chime in too, but CPU runs (pm-cpu here, although gnu) should not vary this much from one run to another.

If you put in call t_startf('timer_name') + call t_stopf('timer_name') before and after the new additions and look at run/timing/model_timing[.00, _stats], do you observe corresponding timing increases?

Are these Ly2-long tests the only tests that show TPUTCOMP differences: e.g.

  • SMS_Ld20.f45_f45.IELMFATES.elm-fates_[eca,rd,satphen] ?

You could try the tests that failed TPUTCOMP on Chrysalis+intel, to rule out a performance regression.

@rljacob rljacob added the Land label Apr 19, 2023
@rljacob rljacob requested review from peterdschwartz, evasinha and dmricciuto and removed request for peterdschwartz April 19, 2023 23:07
@rljacob
Copy link
Member

rljacob commented Apr 19, 2023

I think pm has had a lot of variation while they work on the network.

@rgknox
Copy link
Contributor Author

rgknox commented Apr 20, 2023

@amametjanov those two ly2 tests are the only ones showing tputcomp differences

Copy link
Contributor

@evasinha evasinha left a comment

Choose a reason for hiding this comment

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

The changes in the PR look good and will not make any modifications to the crop simulations.

@bishtgautam
Copy link
Contributor

@rgknox Is this BFB?

@bishtgautam
Copy link
Contributor

@rgknox Just saw your comment above regarding test differences.

@bishtgautam bishtgautam added the non-BFB PR makes roundoff changes to answers. label Apr 28, 2023
bishtgautam added a commit that referenced this pull request May 15, 2023
This set of changes refactors the logic behind N and P supplementation in land bgc.
Also, ecosystem (free-living) N fixation was enabled for fates runs.

[non-BFB]
@bishtgautam bishtgautam merged commit 28feac5 into E3SM-Project:master May 16, 2023
@rljacob rljacob added the FATES label Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FATES Land non-BFB PR makes roundoff changes to answers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants