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

Feature/psturm/kpp standalone interface #9

Open
wants to merge 16 commits into
base: geos/develop
Choose a base branch
from

Conversation

obin1
Copy link
Member

@obin1 obin1 commented Mar 27, 2024

Name and Institution (Required)

Name: Obin Sturm
Institution: University of Southern California

Confirm you have reviewed the following documentation

Describe the update

The initial version of KPP Standalone Interface is used to output the full chemical state of targeted grid cells in 3D runs. Specific grid cells can be configured using a YAML file at run time, but the write output functionality can be used more flexibly in fullchem if output should be written under other conditions (e.g. integrator convergence issues or specific meteorological conditions).

Expected changes

These changes are zero diff to model output, but allow for a new type of model output containing the full chemical state (rate constants and concentrations) needed to replicate chemistry in a specified grid cell using the KPP Standalone chemical integrator.

Reference(s)

None

Related Github Issue(s)

None

@obin1 obin1 added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Mar 27, 2024
@lizziel
Copy link
Collaborator

lizziel commented Mar 27, 2024

Hi @obin1, @christophkeller, et al. - How involved would you like me to be in reviewing this? We want to make sure we don't diverge too much from the standard model. Is this update something that should go into the standard model first and then feed back to geos/develop in a version updates?

@christophkeller
Copy link

Hi @obin1, @christophkeller, et al. - How involved would you like me to be in reviewing this? We want to make sure we don't diverge too much from the standard model. Is this update something that should go into the standard model first and then feed back to geos/develop in a version updates?

We could go that route. @obin1 made only a few updates, and we might just want to cherry-pick his two commits (690d6c2, 2949d61) and bring those into the standard model or geos/develop.
The issue right now is that geos/develop is still behind the standard model, while branch geos/latest_gcc is much closer to the standard model. @obin1 made his modifications off branch geos/latest_gcc, but the PR is going into geos/develop`. If we first sync these two branches, the PR becomes trivial. I can discuss this also with @viral211.

@obin1 obin1 marked this pull request as ready for review April 15, 2024 15:17
Copy link
Collaborator

@lizziel lizziel left a comment

Choose a reason for hiding this comment

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

I am testing this out in the dev version of GEOS-Chem 14.4.0. I will post comments as I come across them.

run/kpp_standalone_interface.yml Outdated Show resolved Hide resolved
GeosCore/kpp_standalone_interface.F90 Outdated Show resolved Hide resolved
@lizziel
Copy link
Collaborator

lizziel commented Apr 26, 2024

The write output functionality can be used more flexibly in fullchem if output should be written under other conditions (e.g. integrator convergence issues or specific meteorological conditions).

Ideally there would be a run-time switch somewhere to turn on these sorts of debug options. Are you planning to add that?

@obin1
Copy link
Member Author

obin1 commented Apr 26, 2024

The write output functionality can be used more flexibly in fullchem_mod.F90 if output should be written under other conditions (e.g. integrator convergence issues or specific meteorological conditions).

Ideally there would be a run-time switch somewhere to turn on these sorts of debug options. Are you planning to add that?

I'm envisioning this as a more general tool, and different use cases would need to add their own logic on when to turn on the writing functionality. One example is around lines 1220-1250 of fullchem_mod.F90 in the version that's in this pull request, where the full chemical state is printed and could instead use the KPP standalone interface. I imagine there could be other use cases that would require different run time switches in the future.

@obin1
Copy link
Member Author

obin1 commented Apr 29, 2024

Hi all, we're still ironing out a few things with this PR. There's a few cases where the KPP Standalone doesn't replicate the 3D results, I've seen this problem when the initial timestep in an input file is equal to the operator splitting timestep. I'll update this thread when the PR is ready for review and potential merge.

@obin1
Copy link
Member Author

obin1 commented May 15, 2024

@christophkeller and @viral211, I've finished the validation for the KPP Standalone -- @msl3v and I think the very few cases that aren't replicated are just from round-off error, and the differences are fairly minor. I think this PR is now ready for review!

Copy link
Member Author

@obin1 obin1 left a comment

Choose a reason for hiding this comment

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

Resolving all threads, we added all the changes @lizziel suggested except for the fort.* issue, which isn't showing up on Discover. We did some additional verification in the KPP Standalone branch and found that the results are almost all replicated, except for a small number of files with small differences in integrator parameters (final timestep and number of internal steps)

@viral211 viral211 requested review from lizziel and removed request for christophkeller May 22, 2024 17:50
@lizziel
Copy link
Collaborator

lizziel commented Sep 16, 2024

@obin1, what is the status of this PR? As I just commented in #19, I wonder if you could make this a PR to geoschem/geos-chem rather than GEOS-ESM/geos-chem so that we can bring it into the standard model before merging into the geos/develop.

@lizziel
Copy link
Collaborator

lizziel commented Sep 16, 2024

Tagging @yantosca

@lizziel
Copy link
Collaborator

lizziel commented Sep 26, 2024

I created a draft PR to bring this into the standard model. See geoschem#2482. @yantosca will be testing.

@obin1
Copy link
Member Author

obin1 commented Sep 26, 2024

Hi Lizzie, sorry for the late response -- my group does not yet have GEOS-Chem in house, so I wasn't sure about the best way to make the pull request and hadn't found a good alternative yet. I saw now that you made the pull request to the standard model, thanks!

By the way, I think I addressed all the changes you requested in the initial review, but don't know how to turn that flag off on the Github browser.

@yantosca
Copy link

yantosca commented Oct 8, 2024

@obin1 @lizziel: I have made several updates to the KPP standalone interface at geoschem#2482. Please feel free to comment further and suggest changes. I was able to add a master on/off switch, as well as adding start/end dates in the YAML file to determine the window when model state should be archived to disk. You can set that to a large period (i.e. 1900..2100) to archive on each timestep or restrict it to the end of a model run (e.g. 20190701 004000 to 20190702 00000,

@obin1
Copy link
Member Author

obin1 commented Oct 9, 2024

@yantosca thanks for adding the timestep option! We had envisioned that as something that could wait for a subsequent version, but this is a really useful addition and great to have already.

@lizziel
Copy link
Collaborator

lizziel commented Nov 21, 2024

This update is now merged into the standard model branch dev/no-diff-to-benchmark. It will be released as part of 14.5.1: geoschem#2482.

@lizziel
Copy link
Collaborator

lizziel commented Nov 21, 2024

@obin1, shall I close this PR since I am bringing 14.5.1 into GEOS? It is not yet released but I will merge in what is available now (to GEOS-ESM geos-chem branch 'geos/latest_gcc'), which includes this update.

@obin1
Copy link
Member Author

obin1 commented Nov 21, 2024

Yes, thank you @lizziel! I think we can go ahead and close this PR. Great to see that it's making it into geos/latest_gcc.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants