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

GDAL_NoData geotiff tag should be a string, not float #1335

Merged
merged 3 commits into from
May 3, 2024

Conversation

WeatherGod
Copy link
Contributor

@WeatherGod WeatherGod commented Apr 19, 2024

Description of Changes

When greyscale is false, the data is scaled to the range of 1-255 before being written to the geotiff. The encoding value of 0 is reserved for missing/nodata values. The missing value is encoded in the GDAL_NoData geotiff tag, but we've been encoding it incorrectly all this time. This PR corrects that, as well as expands the geotiff tests to exercise both cases of greyscale being True and False.

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"

@WeatherGod
Copy link
Contributor Author

Failing test is unrelated as it pertains to zarr, and it might be transient?

@ethanrd
Copy link
Member

ethanrd commented Apr 19, 2024

Hi @WeatherGod - What versions of the GeoTiff Specification (1.0* or 1.1) are you targeting? (I'm not certain what netCDF-java currently supports but suspect it originally targeted 1.0.) It looks like GDAL supports both GeoTIFF 1.0 and 1.1, defaulting to 1.0 (see GEOTIFF_VERSION in GDAL docs Configuration Options section).

I'm not familiar with the differences. From the GDAL docs, maybe just "clear[ing up] ambiguities and fix[ing] inconsistencies". Either way, it's probably best to reference the specification documents than the GDAL documentation to ensure broad interoperability.

[*] FYI There's an illegible TOC button/image at the top of the v1.0 page that, embarrassingly, took me awhile to find.

@WeatherGod
Copy link
Contributor Author

Actually, neither. GDAL_NoData is an extended TIFF tag that has been widely adopted. So much so that it is recognized by the Library of Congress: https://www.loc.gov/preservation/digital/formats/content/tiff_tags.shtml. It says it is ASCII encoded.

@WeatherGod
Copy link
Contributor Author

By the way, I've also come across another set of bugs related to the encoding of the IFDEntry tags. It looks like it doesn't handle unsigned bytes correctly. None of that is tested (until my other PR exposed it).

@WeatherGod
Copy link
Contributor Author

As for the IFDEntry bug, I can't exercise it explicitly with the current API, so I'm just going to have to punt that and have it get fixed by my other PR that exercises it by saving unsigned bytes data. Let me know if you need anything else for this PR.

@ethanrd
Copy link
Member

ethanrd commented Apr 19, 2024

Sorry, I was trying to ask a more general question beyond the GDAL_NoData attribute/tag but didn't find a more general issue to comment on.

Support for "official" standards is a high-level goal for netCDF-Java (and Unidata more generally). Of course, full support for standards isn't always possible and community/implementation conventions are often needed (in addition to standards or as the main focus) for the broadest level of interperability (or simply because that's what gets implemented).

My question was, I guess, mainly about what specification you are using as a reference in your work on the netCDF-Java GeoTIFF code. It sounds like you are using GDAL and LoC GeoTIFF documentation as your main specification. So, I think I have my answer.

Thanks for all your work and contributions!

@tdrwenski tdrwenski merged commit 12d0b71 into Unidata:maint-5.x May 3, 2024
10 checks passed
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

3 participants