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

Skip pob=0.0 in ACARS height fix #259

Merged
merged 6 commits into from
Nov 12, 2024

Conversation

gmao-msienkie
Copy link
Contributor

Check that pressure is greater than zero before calculating new standard atmosphere height for ACARS data
(pob=0.0 is an incorrect obs anyway)

@gmao-msienkie gmao-msienkie requested a review from a team as a code owner August 10, 2023 21:55
@github-actions
Copy link

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:

@gmao-msienkie gmao-msienkie self-assigned this Aug 10, 2023
@gmao-msienkie gmao-msienkie added the 0 diff The changes in this pull request have verified to be zero-diff with the target branch. label Aug 10, 2023
@gmao-msienkie
Copy link
Contributor Author

I think that my branch is up to date with GEOSadas-5.29.4-p4 (for sweeper and GEOS-IT).
The sweeper needs this change now and GEOSIT will need it really soon.
If you want/need to update for 'develop' that's fine by me.

I have tested to see that the code change gives zero-diff running a test case.
If it is run with a file with ACARS data with pob=0 it will not be zero diff because it will not crash = but all of the good observations will have the same correction as before the modification.

@rtodling
Copy link
Collaborator

rtodling commented Mar 5, 2024

Meta - thanks for the change here. I have to say that I am not sure how changes to GEOS-IT, and much less, to M21C (and sweeper) are being handled. I am under the impression that GEOS-IT has already suffered multiple updates/changes and nothing has made it into the repo.

As for M21C ... I imagine this can be coordinated w/ Amal who's still finalizing changes for that purpose.

What I can do is:

  1. make sure this gets to develop - easy - I will approve this as soon as I finish typing this.
  2. and I have have FP patched to incorporate this as well - though I have just issued a patch to FP and much prefer not to have to do another one ...I'll see w/ Rob and Mark if it's till time to push the tag.

@gmao-msienkie
Copy link
Contributor Author

I just checked in the pull request "R21C DAS Candidate #268" and it looks like this change (as well as many others) is already in that branch. I believe I created the change when the sweeper encountered the problem and crashed and Rob recompiled with the patch prior to GEOSIT reaching the problem at 20150304 18z. At the time both Rob and Amal agreed it was better to modify the code and recompile rather than just fix the problem input file.
I don't think it is necessary to modify FP since it doesn't use the preprocessing QC. It is also not needed for earlier systems like MERRA2 since the code to recalculate standard atmosphere height was not included in that version. Recalculating standard atmosphere height was only needed after the NRL aircraft QC was adopted to correct the heights in aircraft data processed by an earlier version of the PREPDATA processing.
This can be added now to develop unless you're going to bring in the fix with other changes from #268 - either way we can close out this ticket.

@gmao-msienkie
Copy link
Contributor Author

There is a Trac ticket (2304) for GEOS-IT where Rob implemented this fix.

@rtodling
Copy link
Collaborator

rtodling commented Mar 6, 2024

There is a Trac ticket (2304) for GEOS-IT where Rob implemented this fix.

I understand Meta - I know about the trak tickets, but things still need to make their way into the git repo.

@gmao-msienkie
Copy link
Contributor Author

The change was downloaded into the GEOSadas-5.29.4-p4 build in ~dao_ops and recompiled.
I don't know if there is a process where Rob is updating a Github branch with patches for GEOSIT the way we used to have branches and tags in CVS. It should be easy enough to do.

@rtodling
Copy link
Collaborator

rtodling commented Nov 5, 2024

The change was downloaded into the GEOSadas-5.29.4-p4 build in ~dao_ops and recompiled. I don't know if there is a process where Rob is updating a Github branch with patches for GEOSIT the way we used to have branches and tags in CVS. It should be easy enough to do.

There is no process other than create a PR w/ the pointer to each branch we mean the change to go into. I change like this would have at least two (of three) PRs, one pointing to develop, another to GEOS-IT and perhaps a third to R21C - the last two are existing branches in the analysis and GEOSadas repos ... sorry - it's cumbersome but git is not very smart about put the same change in multiple branches!

@rtodling
Copy link
Collaborator

rtodling commented Nov 5, 2024

@mathomp4 it's not clear why the CI keeps failing here for such a simple change ... could it be related to the fact that GEOSadas still doesn't have a changelog?

@mathomp4
Copy link
Member

mathomp4 commented Nov 5, 2024

@mathomp4 it's not clear why the CI keeps failing here for such a simple change ... could it be related to the fact that GEOSadas still doesn't have a changelog?

@rtodling Until #297 is folded into develop (and any branch based on develop), the CI for GEOSadas will fail. This is due to the problems with f2py and our CI environment.

Copy link
Collaborator

@rtodling rtodling left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@rtodling rtodling merged commit 4d0461b into develop Nov 12, 2024
2 of 3 checks passed
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.

3 participants