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

Remove min/max in computing CellType for GDALDataset #3150

Open
wants to merge 1 commit into
base: master
from

Conversation

@vpipkt
Copy link
Member

vpipkt commented Nov 8, 2019

Overview

GDAL's type system is narrower than GT
GDALDataType.GDT_Byte maps to UByte* CellTypes only depending on NoData

In GDALUtils.dataTypeToCellType, only get unsigned Byte by passing in appropriate min and max. This is to avoid breaking the existing call behavior.

Some gitter discussion on the topic here

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • Module Hierarcy updated, if necessary
  • docs guides update, if necessary
  • New user API has useful Scaladoc strings
  • Unit tests added for bug-fix or new feature

Closes #3148

…aset

    GDAL's type system is narrower than GT
    GDALDataType.GDT_Byte maps to UByte* only depending on NoData

    In GDALUtils.dataTypeToCellType, only get unsigned Byte by passing in appropriate min and max.

Signed-off-by: Jason T. Brown <jason@astraea.earth>
@vpipkt

This comment has been minimized.

Copy link
Member Author

vpipkt commented Nov 8, 2019

@pomadchin says on gitter: can you cover it with test? so we would know that GDALRasterSource behaves the same way GeoTiffRasterSource does

@vpipkt

This comment has been minimized.

Copy link
Member Author

vpipkt commented Nov 8, 2019

Unsure why the CI build is failing. Looks like one is marked as fail but logs look clean? Another has a failed dependency resolution :-(

@pomadchin

This comment has been minimized.

Copy link
Member

pomadchin commented Nov 8, 2019

Restarted the build; also this PR would probably have to be double checked by the RasterFoundry team

@vpipkt

This comment has been minimized.

Copy link
Member Author

vpipkt commented Nov 8, 2019

Feel free to assign the appropriate members from the community to review it.

@pomadchin

This comment has been minimized.

Copy link
Member

pomadchin commented Nov 8, 2019

@vpipkt btw so what about tests? This PR in general makes sense, but I remember that we introduced these changes to make it behave like GeoTiffRasterSource. I would like to see smth like reading different Byte TIFFs with GDALRasterSource and GeoTiffRasterSource and compare the results.

For the context:
the original issue was raster-foundry/raster-foundry#4442 and it was addressed here geotrellis/geotrellis-gdal#40

I also would like to mention @notthatbreezy since he can be intrested in this change and mb will help us to test it out.

@vpipkt

This comment has been minimized.

Copy link
Member Author

vpipkt commented Nov 11, 2019

@pomadchin very helpful, I am looking into the original issue and PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.