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

nread bug in read_radar.f90 #738

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

ShunLiu-NOAA
Copy link
Contributor

@ShunLiu-NOAA ShunLiu-NOAA commented Apr 24, 2024

Description
In read_radar.f90, the value of nread should be "nsuper2_kept" and shouldn't be reset to zero after processing TDR data.

Resolves #737

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

@ShunLiu-NOAA ShunLiu-NOAA changed the title The value of nread should be nsuper2_kept and shouldn't be reset to z… nread bug in read_radar.f90 Apr 24, 2024
@JingCheng-NOAA
Copy link
Contributor

There is already an if condition if (trim(infile) /= 'tldplrbufr' .and. trim(infile) /= 'tldplrso') then line 368 to line 2152 to exclude those two types of data. Do we need to check it again on line 2155 if (trim(infile) == 'tldplrbufr' .and. trim(infile) == 'tldplrso') then?

src/gsi/read_radar.f90 Outdated Show resolved Hide resolved
@ShunLiu-NOAA
Copy link
Contributor Author

There is already an if condition if (trim(infile) /= 'tldplrbufr' .and. trim(infile) /= 'tldplrso') then line 368 to line 2152 to exclude those two types of data. Do we need to check it again on line 2155 if (trim(infile) == 'tldplrbufr' .and. trim(infile) == 'tldplrso') then?

Yes. line 368 is to process level2 radar data. Without line255, nread is reset to zero even there is no TDR data. This is one of reason that the value of nread is incorrect after read_radar.

@JingCheng-NOAA
Copy link
Contributor

There is already an if condition if (trim(infile) /= 'tldplrbufr' .and. trim(infile) /= 'tldplrso') then line 368 to line 2152 to exclude those two types of data. Do we need to check it again on line 2155 if (trim(infile) == 'tldplrbufr' .and. trim(infile) == 'tldplrso') then?

Yes. line 368 is to process level2 radar data. Without line255, nread is reset to zero even there is no TDR data. This is one of reason that the value of nread is incorrect after read_radar.

Understand. Then I have no issue with this change.

Copy link
Contributor

@XuLu-NOAA XuLu-NOAA left a comment

Choose a reason for hiding this comment

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

nread should be the total # of obs that read in (corresponds to nsuper2_in), not the actual # of obs that is kept. Additionally, it makes more sense to use nread=nread+1 inside the superobs loop near nsuper2_in instead of outside the loop.
Additionally, for the if statement, even if using or in the if condition. If the infile is neither 'tldplrbufr' nor 'tldplrso', then integers like noutside would not be initialized (e.g. L2861).

src/gsi/read_radar.f90 Show resolved Hide resolved
src/gsi/read_radar.f90 Outdated Show resolved Hide resolved
@ShunLiu-NOAA
Copy link
Contributor Author

Regression tests passed on both Hera and WCOSS2.

Hera:
Test project /scratch2/NCEPDEV/fv3-cam/noscrub/Shun.Liu/gsi/GSI/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 487.53 sec
2/7 Test #3: rrfs_3denvar_glbens .............. Passed 489.97 sec
3/7 Test #7: global_enkf ...................... Passed 807.93 sec
4/7 Test #2: rtma ............................. Passed 968.86 sec
5/7 Test #6: hafs_3denvar_hybens .............. Passed 1100.45 sec
6/7 Test #5: hafs_4denvar_glbens .............. Passed 1415.11 sec
7/7 Test #1: global_4denvar ................... Passed 1866.52 sec

100% tests passed, 0 tests failed out of 7

WCOSS2
Test project /lfs/h2/emc/da/noscrub/Shun.Liu/GSI/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 484.44 sec
2/7 Test #3: rrfs_3denvar_glbens .............. Passed 607.88 sec
3/7 Test #2: rtma ............................. Passed 970.13 sec
4/7 Test #6: hafs_3denvar_hybens .............. Passed 1214.62 sec
5/7 Test #5: hafs_4denvar_glbens .............. Passed 1334.32 sec
6/7 Test #7: global_enkf ...................... Passed 1411.76 sec
7/7 Test #1: global_4denvar ................... Passed 1735.00 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 1735.02 sec

@hu5970 hu5970 merged commit a3a2633 into NOAA-EMC:develop Apr 30, 2024
4 checks passed
hu5970 added a commit that referenced this pull request May 7, 2024
This PR is to bring read_radar bug fix (#738) committed in the develop branch to RRFS release branch.
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.

nread bug in read_radar.f90
4 participants