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

[5.x]: Adding the ability to save geotiffs with a palette #1066

Open
wants to merge 13 commits into
base: maint-5.x
Choose a base branch
from

Conversation

WeatherGod
Copy link
Contributor

@WeatherGod WeatherGod commented Aug 22, 2022

Description of Changes

Add support to save out a geotiff with supplied colortable

PR Checklist

  • Indicate the version associated with this PR in the Title
    (e.g. "[5.x]: This is my PR title")
  • Link to any issues that the PR addresses
  • Add labels, especially if the PR should be ported to other versions
    (these labels start with "port: ")
  • Open as a draft PR
    until ready for review
  • Make sure GitHub tests pass
  • Mark PR as "Ready for Review"

@CLAassistant
Copy link

CLAassistant commented Aug 22, 2022

CLA assistant check
All committers have signed the CLA.

@WeatherGod
Copy link
Contributor Author

This is my first significant chunk of Java code, so don't assume I did any of this correctly.

The reason for this submission and its design is that I plan to also make a submission to TDS's WCS service to add a new Geotiff_Palette format that would then respect a dataset's "flag_values" and "flag_colors" attributes, much like how ncWMS does right now. We could also respect supplied styling options this way, too.

@WeatherGod
Copy link
Contributor Author

And, this is partly in response to #1052

@WeatherGod
Copy link
Contributor Author

I am just about done with a set of revisions that I think makes things more sane, but I needed to write a coerceInt() and a coerceByte() private methods to coerce an arbitrary Array into ArrayInt or ArrayByte copies, respectively. I feel like that is some sort of NiH syndrome. Is there already a "correct" way to do what in numpy would be like an a.astype(int)?

@tdrwenski
Copy link
Member

Hi @WeatherGod, are you still working on this PR?

@WeatherGod
Copy link
Contributor Author

WeatherGod commented Apr 11, 2024 via email

@tdrwenski
Copy link
Member

Okay, thanks! Will try to have a look at your comments about the possible bug. To see the style issues you can run ./gradlew spotlessCheck or to automatically fix them you can run ./gradlew spotlessApply

WeatherGod and others added 8 commits May 9, 2024 13:52
* Fixing some data-handling issues
* Make the greyscale mode completely separate from the colortable handling
* Added the ability to specify the output data type
* Particularly noticeable for sample min/max metadata
* Expand unit tests for geotiff writing
* Reduce code duplication
@WeatherGod
Copy link
Contributor Author

Crazy thought,, but what if we allowed color tables to work with greyscale==true? Right now, as coded, greyscale mode scales the given data to the range of 1-255 and encodes missing data as 0, effectively unsigned bytes, while the "colortable" mode doesn't do any scaling at all, and only works if the data type is unsigned bytes. But, it is getting tedious to document and code checks that the colortable can only be used when greyscale is false. Why not allow color tables to be used for greyscale mode?

It might be easier to explain the various features available as "greyscale normalizes to UBYTE", "color table works only for UBYTEs", and "Non-greyscale FLOATs encodes missing data as data minimum minus one".

Thoughts?

@WeatherGod
Copy link
Contributor Author

Now that I've actually implemented that idea, I really like it. I'm running with it. It also addresses several other questions I had in my mind about possible use-cases and whether or not the setColorTable() was an anti-pattern.

* Make it possible to use colortable regardless of greyscale mode, so long as it is UBYTE
* Fix some of the coercion code
* Added more tests
* Improved documentation and input checking
@WeatherGod WeatherGod marked this pull request as ready for review May 10, 2024 20:40
@WeatherGod
Copy link
Contributor Author

Test failures seem to be unrelated?

@tdrwenski
Copy link
Member

Test failures seem to be unrelated?

Hi @WeatherGod, I think I know the issue there, should have a fix shortly!

@WeatherGod
Copy link
Contributor Author

Is there anyplace where I should add some narrative docs explaining the changes to the geotiff module?

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

4 participants