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/gaas fix for amip emissions #282

Merged
merged 4 commits into from
Sep 26, 2024

Conversation

vbuchard
Copy link
Contributor

@vbuchard vbuchard commented Sep 6, 2024

When the model runs in replay with GAAS turned on and AMIP emissions chosen, the model crashes after several days with a segmentation fault. AOD becoming completely off.
This PR updates the GAAS_GridComp_Extdata.yaml in AMIP/ similar to the one in OPS.

Fixes #281

@vbuchard vbuchard added the bugfix This fixes a bug label Sep 6, 2024
@vbuchard vbuchard requested a review from bena-nasa September 6, 2024 18:42
@vbuchard vbuchard requested review from a team as code owners September 6, 2024 18:42
Copy link

github-actions bot commented Sep 6, 2024

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found: bugfix

1 similar comment
Copy link

github-actions bot commented Sep 6, 2024

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found: bugfix

Copy link

github-actions bot commented Sep 6, 2024

Label error. Requires at least 1 of: 0 diff, 0 diff trivial, Non 0-diff, 0 diff structural, 0-diff trivial, Not 0-diff, 0-diff, automatic, 0-diff uncoupled. Found: bugfix

@vbuchard vbuchard added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Sep 6, 2024
@vbuchard
Copy link
Contributor Author

Just a note: this file will need to be updated if the offset is no longer 450s (a match of the heartbeat_dt in cap.rc). @bena-nasa has open a ticket to have a more flexible solution in ExtData.

@mathomp4
Copy link
Member

Just a note: this file will need to be updated if the offset is no longer 450s (a match of the heartbeat_dt in cap.rc). @bena-nasa has open a ticket to have a more flexible solution in ExtData.

Ooh. Yeah. In GEOSgcm v12 this can really not be true. The heartbeat is resolution dependent from like 1800 s to 225 or so.

@mmanyin
Copy link
Contributor

mmanyin commented Sep 25, 2024

Just a note: this file will need to be updated if the offset is no longer 450s (a match of the heartbeat_dt in cap.rc). @bena-nasa has open a ticket to have a more flexible solution in ExtData.

Ooh. Yeah. In GEOSgcm v12 this can really not be true. The heartbeat is resolution dependent from like 1800 s to 225 or so.

When we get to that point, I would recommend we use a token like HEARTBEAT in the yaml file that gets replaced by the run script.

@mmanyin
Copy link
Contributor

mmanyin commented Sep 25, 2024

Do we really want fail_on_missing_file to be FALSE by default? Won't this lead to confusion if the file path is wrong, but the user assumes that all is working properly? What would be the down-side of fail_on_missing_file = TRUE?

@mathomp4
Copy link
Member

Do we really want fail_on_missing_file to be FALSE by default? Won't this lead to confusion if the file path is wrong, but the user assumes that all is working properly? What would be the down-side of fail_on_missing_file = TRUE?

I think @bena-nasa knows why that is there...and I think it needs to be?

@bena-nasa
Copy link
Collaborator

bena-nasa commented Sep 26, 2024

@mmanyin, @vbuchard we probably don't want fail_on_missing_file in AMIP mode is my best guess. However, that is 100% necessary when GAAS is run in OPS since they run the forecast as a part of the same run where the assimilation happens.

So we might want to eliminate that in the AMIP mode

@bena-nasa
Copy link
Collaborator

@mmanyin with regards to the HEARTBEAT issue. That's also on the to do list for ExtData

@mmanyin
Copy link
Contributor

mmanyin commented Sep 26, 2024

@bena-nasa Thanks for the clarifications. I will go ahead and approve the PR, with the big caveat to set fail_on_missing_file to TRUE if the model is running in non-forecast mode.

Copy link
Contributor

@mmanyin mmanyin left a comment

Choose a reason for hiding this comment

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

This PR is a step in the right direction. It depends on the proper assignment of REPLAY_ANA_EXPID and REPLAY_ANA_LOCATION in AGCM.rc, and some clever code in the run script, but it works. Two caveats, as mentioned in the discussion: PT450S will need to be changed if the timestep changes. And the 'fail_on_missing_file' flag for each export should probably be set to TRUE for hindcast simulations, to help guard against inadvertent mistakes.

@mmanyin mmanyin merged commit 453771c into develop Sep 26, 2024
14 of 15 checks passed
@mmanyin mmanyin deleted the feature/GAAS_fix_for_AMIP_emissions branch September 26, 2024 15:31
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.

GAAS broken when AMIP emissions chosen
4 participants