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

CrIS viirs bug fix #708

Merged
merged 2 commits into from
Feb 27, 2024
Merged

Conversation

wx20jjung
Copy link
Contributor

@wx20jjung wx20jjung commented Feb 26, 2024

DUE DATE for merger of this PR into develop is 4/8/2024 (six weeks after PR creation).

Description
The new cloud and aerosol detection software (CADS) requires the corresponding imager CRTM coefficient files. There are satellite identifier mis-matches for CrIS-VIIRS instruments on NOAA-20 and NOAA-21. The VIIRS files are named J1 and J2 for NOAA-20 and -21 respectively. Eventually the CRTM group will develop the correct named files. When this happens read_cris should transition to the new file names without incident. The current code will fail.

The code changes in read_cris will transition between j2 and n21 without incident. This was tested on S4 and Hera. The same logic was put in for J1 and n20 but can't be tested. This PR will fix #706.

Fixes #706

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

Ctests were conducted on Hera and Orion.

Hera stats:
Start 1: [=[global_4denvar]=]
Start 2: [=[rtma]=]
Start 3: [=[rrfs_3denvar_glbens]=]
Start 4: [=[netcdf_fv3_regional]=]
Start 5: [=[hafs_4denvar_glbens]=]
Start 6: [=[hafs_3denvar_hybens]=]
Start 7: [=[global_enkf]=]
1/7 Test #3: [=[rrfs_3denvar_glbens]=] ........ Passed 1210.74 sec
2/7 Test #4: [=[netcdf_fv3_regional]=] ........ Passed 1510.34 sec
3/7 Test #7: [=[global_enkf]=] ................ Passed 1689.43 sec
4/7 Test #6: [=[hafs_3denvar_hybens]=] ........ Passed 1826.83 sec
5/7 Test #2: [=[rtma]=] ....................... Passed 1996.78 sec
6/7 Test #5: [=[hafs_4denvar_glbens]=] ........ Passed 2187.12 sec
7/7 Test #1: [=[global_4denvar]=] ............. Passed 2213.32 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 2213.33 sec

Orion stats:
Start 1: [=[global_4denvar]=]
Start 2: [=[rtma]=]
Start 3: [=[rrfs_3denvar_glbens]=]
Start 4: [=[netcdf_fv3_regional]=]
Start 5: [=[hafs_4denvar_glbens]=]
Start 6: [=[hafs_3denvar_hybens]=]
Start 7: [=[global_enkf]=]
1/7 Test #4: [=[netcdf_fv3_regional]=] ........ Passed 666.36 sec
2/7 Test #7: [=[global_enkf]=] ................ Passed 683.18 sec
3/7 Test #3: [=[rrfs_3denvar_glbens]=] ........ Passed 730.58 sec
4/7 Test #2: [=[rtma]=] ....................... Passed 1088.72 sec
5/7 Test #6: [=[hafs_3denvar_hybens]=] ........ Passed 1399.00 sec
6/7 Test #1: [=[global_4denvar]=] ............. Passed 1623.90 sec
7/7 Test #5: [=[hafs_4denvar_glbens]=] ........***Failed 1639.01 sec

86% tests passed, 1 tests failed out of 7

Total Test time (real) = 1639.03 sec

The failure on orion is a time threshold test:
The runtime for hafs_4denvar_glbens_loproc_updat is 389.841067 seconds. This has exceeded maximum allowable threshold time of 389.025272 seconds,
resulting in Failure time-thresh of the regression test.

@RussTreadon-NOAA
Copy link
Contributor

Thank you @ADCollard for your review and approval.

@wx20jjung ,thank you for running ctests on Hera and Orion. I'll run the tests on WCOSS2. We need one additional peer reviewer. Let me know who you want and we can and him/her to the reviewer list.

@wx20jjung
Copy link
Contributor Author

I asked Dave Huber. He will do the 2nd review.
Jim

@RussTreadon-NOAA
Copy link
Contributor

I asked Dave Huber. He will do the 2nd review. Jim

Thanks. I added @DavidHuber-NOAA as a reviewer.

Copy link
Collaborator

@DavidHuber-NOAA DavidHuber-NOAA left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the catch @wx20jjung!

@RussTreadon-NOAA
Copy link
Contributor

WCOSS2 ctests
wx20jjung:CrIS-VIIRS_bug at 76f22b1 was installed on Dogwood along with develop at 7c4a571. ctests were run with the following results

Test project /lfs/h2/emc/da/noscrub/russ.treadon/git/gsi/pr708/build
    Start 1: global_4denvar
    Start 2: rtma
    Start 3: rrfs_3denvar_glbens
    Start 4: netcdf_fv3_regional
    Start 5: hafs_4denvar_glbens
    Start 6: hafs_3denvar_hybens
    Start 7: global_enkf
1/7 Test #4: netcdf_fv3_regional ..............   Passed  483.07 sec
2/7 Test #3: rrfs_3denvar_glbens ..............   Passed  485.66 sec
3/7 Test #7: global_enkf ......................   Passed  611.07 sec
4/7 Test #2: rtma .............................   Passed  969.13 sec
5/7 Test #6: hafs_3denvar_hybens ..............   Passed  1093.05 sec
6/7 Test #5: hafs_4denvar_glbens ..............   Passed  1342.07 sec
7/7 Test #1: global_4denvar ...................   Passed  1442.62 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 1442.68 sec

@RussTreadon-NOAA
Copy link
Contributor

@wx20jjung : Quick comment: All tests pass on Dogwood, Hera, and Orion with (this PR) or without (develop) the bug fix. Since our ctests yield Passed this suggests that our current suite of ctests do not exercise the code in question. We should update our ctests to exercise the corrected code. Doing so is not a requirement for this PR but it's something we don't want to forget. ctests should not be static. They need periodic updates, too.

Copy link
Contributor

@RussTreadon-NOAA RussTreadon-NOAA left a comment

Choose a reason for hiding this comment

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

Approve.

@RussTreadon-NOAA
Copy link
Contributor

@wx20jjung : Quick comment: All tests pass on Dogwood, Hera, and Orion with (this PR) or without (develop) the bug fix. Since our ctests yield Passed this suggests that our current suite of ctests do not exercise the code in question. We should update our ctests to exercise the corrected code. Doing so is not a requirement for this PR but it's something we don't want to forget. ctests should not be static. They need periodic updates, too.

OK. I think I see what's going on. Looking at the issue this PR future-proofs the code. It's not fixing a current bug. It's looking ahead to when the current code will fail as is. Is this correct?

@RussTreadon-NOAA
Copy link
Contributor

@wx20jjung : Quick comment: All tests pass on Dogwood, Hera, and Orion with (this PR) or without (develop) the bug fix. Since our ctests yield Passed this suggests that our current suite of ctests do not exercise the code in question. We should update our ctests to exercise the corrected code. Doing so is not a requirement for this PR but it's something we don't want to forget. ctests should not be static. They need periodic updates, too.

OK. I think I see what's going on. Looking at the issue this PR future-proofs the code. It's not fixing a current bug. It's looking ahead to when the current code will fail as is. Is this correct?

@wx20jjung , given your confirmation that my understanding is correct we can schedule this PR for merger into develop.

@wx20jjung
Copy link
Contributor Author

Russ,

You are correct. This change circumvents a potential problem when the "consistent" crtm file names are available. When I tried this on S4, the gsi crashed, not just read_cris. Similar results on hera. I don't want this possibility to exist in operations.

@RussTreadon-NOAA
Copy link
Contributor

Thanks, @wx20jjung . I'll work with the Handling Review team to schedule merger of this PR into develop. I appreciate your being proactive, especially with regards to operations.

@RussTreadon-NOAA RussTreadon-NOAA merged commit fca6bea into NOAA-EMC:develop Feb 27, 2024
4 checks passed
tsga pushed a commit to tsga/GSI that referenced this pull request Mar 12, 2024
tsga pushed a commit to tsga/GSI that referenced this pull request Mar 12, 2024
tsga pushed a commit to tsga/GSI that referenced this pull request Mar 12, 2024
tsga added a commit to tsga/GSI that referenced this pull request Mar 12, 2024
* origin:
  Revert "Revert "CrIS viirs bug fix (NOAA-EMC#708)""
  Revert "CrIS viirs bug fix (NOAA-EMC#708)"
  CrIS viirs bug fix (NOAA-EMC#708)
tsga added a commit to tsga/GSI that referenced this pull request Mar 12, 2024
… feature/paranc_sfc

* 'feature/paranc_sfc' of https://github.com/tsga/GSI:
  Revert "CrIS viirs bug fix (NOAA-EMC#708)"
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.

VIIRS coefficient file selection bug in read_cris
4 participants