Skip to content

Conversation

@michaeldiener
Copy link
Contributor

Description of Changes

cdm-radial: fix for sigmet volume scan.
Before there were issues with some files from IDEAM Colombia.

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"

@lesserwhirls lesserwhirls added bug Something isn't working radial radial data formats labels Jan 30, 2025
@lesserwhirls lesserwhirls modified the milestones: v5.3, v5.8 Jan 30, 2025
@lesserwhirls
Copy link
Member

Thank you for your contribution! I'd like to get some tests in place to make sure the fixes are covered in the future. If you have a small (<200 KiB or so) sample file, it can be included in the git repository and the tests can live under cdm/radial/src/test/java/ucar/nc2/iosp/sigmet - this setup would run the test against each PR. If the file is larger, we could include it in our test datasets, which are used to run our nightly integration testing. The test would need to be annotated with @Category(NeedsCdmUnitTest.class), and the path to the test dataset would need to be updated to reflect the sample files final location within our test dataset filesystem layout (I can do that, if you write the tests with a bogus path that could be updated by me in a separate PR).

@michaeldiener
Copy link
Contributor Author

The test file that I have is 387.1 kB.
I don't own the copyright, but it is Creative Commons Attribution 4.0 International - not sure this is compatible with test datasets.

Please advise on how to proceed.

@lesserwhirls
Copy link
Member

CC 4.0 Attribution International will work just fine. I think the size is ok to include in the repo, which is a bit nicer since the test will run against every PR. What we'd want is for the file to live under:

cdm/radial/src/test/data/sigmet

and along side of it, create a README file that references the file name and provides the attribution details following the the requirements laid out by the license. I believe this is the license, but you'd want to confirm that. If that is the license, this is what is required:

Attribution — You must give appropriate credit , provide a link to the license, and indicate if changes were made . You may do so in any reasonable manner, but not in any way that suggests the licensor endorses you or your use.

Your test code can live under cdm/radial/src/test/java/ucar/nc2/iosp/sigmet, and you can access the test file using the path TestDir.localTestDataDir + "sigmet/<test-file-name>";. Note that the existing test in that directory requires access to our large test datasets, so it is annotated with @Category(NeedsCdmUnitTest.class) - your test will not use that annotation since the test file will live in the repository :-)

Thank you again!

@michaeldiener
Copy link
Contributor Author

Added the file and the test.

Thanks for explaining how to add the test. Good to know how it works and so I can add tests next time when I fix something. Probably should have done this also last year, but unfortunately I don't have test files for those fixes anymore.

@lesserwhirls
Copy link
Member

No worries, and thank you for your contribution!

@lesserwhirls lesserwhirls merged commit 8b5855c into Unidata:maint-5.x Feb 3, 2025
8 checks passed
@michaeldiener michaeldiener deleted the maint-5.x_sigmetfix branch February 4, 2025 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working radial radial data formats

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants