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

cdm-radial: sigmet fixes #1295

Merged
merged 22 commits into from
Mar 14, 2024
Merged

cdm-radial: sigmet fixes #1295

merged 22 commits into from
Mar 14, 2024

Conversation

michaeldiener
Copy link
Contributor

@michaeldiener michaeldiener commented Jan 27, 2024

  • Support for all data types instead of just the basic 5 (power, reflectivity, width, differential reflectivity)
  • Universal handling for data types instead of each one separately
  • Most new data types are returned as raw values meaning no normalization/decoding done like for the basic 5 - see chapter 4.4
  • This includes support for data types that span more than one byte per bin
  • Added comments and small fixes

Ultimately all these changes add support for IDEAM Colombia files

Addresses the github issue #1292

I worked with this version of the Sigmet/IRIS specification: IRIS-Programming-Guide-M211318EN.pdf
(not attaching it because of copyright)

PR Checklist

  • Link to any issues that the PR addresses
  • Add labels
  • Open as a draft PR
    until ready for review
  • Make sure GitHub tests pass
  • Mark PR as "Ready for Review"

Support for all data types instead of just 5
Universal handling for data types instead of each one separately
Support for data types spanning two bytes instead of one is experimental at this point as I didn't have such a file to test
Most new data types are returned as raw values meaning no normalization like done for total power, reflectivity, velocity, width, differential reflectivity
Added comments and small fixes

Ultimately all these changes add support for IDEAM Colombia files

Addresses the github issue #1292
Code Style... :/
Another fix necessary to support IDEAM Colombia files
In this case not all sweeps of num_sweeps have data, so adapting for this and handling it gracefully. Happens for the radars San_Andres and Corozal.
@haileyajohnson haileyajohnson marked this pull request as draft January 30, 2024 17:20
@haileyajohnson
Copy link
Member

Hi @michaeldiener , thanks for contributing! I've marked this PR as a draft, please set it to ready for review when it's ready!

@michaeldiener michaeldiener marked this pull request as ready for review January 30, 2024 18:01
@michaeldiener
Copy link
Contributor Author

Sorry for another commit today, it was a last minute fix. All done now, thank you.

@michaeldiener michaeldiener marked this pull request as draft January 31, 2024 10:21
@michaeldiener
Copy link
Contributor Author

I'm really sorry, I have encountered another issue. Keeping this as a draft now for a few days to avoid wasting anybody's time.

Data types with more than 1 byte fully supported
A few small fixes
@michaeldiener
Copy link
Contributor Author

@haileyajohnson The test here seems to fail due to a connection issue when I'm not mistaken. This seems to happen in a module totally unrelated to the one I modified. Is this a known issue? How to proceed?

`dap4.test.TestHyrax > test[5: nc4_strings_comp.nc] STANDARD_ERROR
Comparison: vs /github/workspace/dap4/src/test/data/resources/baselinehyrax/nc4_strings_comp.nc.ncdump
>>>> 21 CHANGED FROM
station =
{ "one", "two", "three", "four", "five", "one_b", "two_b", "three_b", "four_b", "five_b", "one_c", "two_c", "three_c", "four_c", "five_c", "one", "two", "three", "four", "five", "one", "two", "three", "four", "five", "one_f", "two_f", "three_f", "four_f", "five_f"
}
scan_line = "r", "r1", "r2", "r3", "r4"
codec_name = "mp3"
lat =
{0, 10, 20, 30, 40, 50}
lon =
{-140, -118, -96, -84, -52}
}
>>>> CHANGED TO
dap4.core.util.DapException: Request failure: 500: http://test.opendap.org/opendap/nc4_test_files/nc4_strings_comp.nc.dap?dap4.checksum=true
>>>> Dap4 Testing: End of differences.

dap4.test.TestHyrax > test[5: nc4_strings_comp.nc] FAILED
java.lang.AssertionError: *** FAIL
at org.junit.Assert.fail(Assert.java:89)
at org.junit.Assert.assertTrue(Assert.java:42)
at dap4.test.TestHyrax.test(TestHyrax.java:186)`

@haileyajohnson
Copy link
Member

@michaeldiener that looks like a flaky test. I'll rerun your tests and if it fails again I'll turn that one off.

@haileyajohnson
Copy link
Member

we're taking a look at the flaky tests but feel free to mark this ready when you want us to review it

@michaeldiener
Copy link
Contributor Author

@haileyajohnson Thank you! I'm going to wait for a few more days, just to make sure everything keeps running smoothly in my own server environment.
Also I'm trying to find more Sigmet IRIS example files, especially ones that contain data types with more than one byte per bin, e.g. DB_DBZ2 Reflectivity (2 byte). This seems harder than anticipated. I sent a support request to Vaisala, they are the ones that create the original Sigmet IRIS software, but not sure they will respond as I'm not one of their customers. Other than that I'm not sure where I can find a file like this.

Fix for 2 byte data types
(was able to find an example file finally)
Fix for reading data from ray for only the number of bins available
@michaeldiener
Copy link
Contributor Author

Found an example of a file I was looking for eventually. All working well now IMHO.

@michaeldiener michaeldiener marked this pull request as ready for review February 5, 2024 09:16
Support for all data types instead of just 5
Universal handling for data types instead of each one separately
Support for data types spanning two bytes instead of one is experimental at this point as I didn't have such a file to test
Most new data types are returned as raw values meaning no normalization like done for total power, reflectivity, velocity, width, differential reflectivity
Added comments and small fixes

Ultimately all these changes add support for IDEAM Colombia files

Addresses the github issue #1292
Code Style... :/
Another fix necessary to support IDEAM Colombia files
In this case not all sweeps of num_sweeps have data, so adapting for this and handling it gracefully. Happens for the radars San_Andres and Corozal.
Data types with more than 1 byte fully supported
A few small fixes
Fix for 2 byte data types
(was able to find an example file finally)
Fix for reading data from ray for only the number of bins available
@haileyajohnson
Copy link
Member

@michaeldiener I pushed a few changes to your branch and I'm gonna run it against our full test suite, but if that passes I'd say it's ready to go. Thanks again for contributing!

@michaeldiener
Copy link
Contributor Author

Please let me go through your changes before merging. At first sight there are a few things I want to have a closer look.

Fixes for change "PR feedback and cleanup" b89cf77
@michaeldiener
Copy link
Contributor Author

@haileyajohnson Thanks for the review!
Unfortunately some of your optimizations introduced 2 new bugs and I have resolved this in my latest commit.

@haileyajohnson
Copy link
Member

Thanks! We're having some technical difficulties with our test data at the moment, so I'm waiting on that to resolve to run our tests :)

@haileyajohnson haileyajohnson merged commit 3fb7e4c into Unidata:maint-5.x Mar 14, 2024
10 checks passed
@michaeldiener michaeldiener deleted the maint-5.x_sigmet branch March 14, 2024 07:18
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.

None yet

2 participants