-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add ability to read tif files #31
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR @csherwood-usgs ! I left a few comments but I have a major question next to that. As far as I understood the doodler app doesn't have any geo dimension, the images that are ought to be labelled don't necessarily have to be aerial images, they could represent anything. With this PR you introduce rioxarray
to read GeoTiff files, which will pull large dependencies like GDAL and PROJ (rioxarray
depends on pyproj
and rasterio
). Is there a plan for the app to become more geo aware? Because if you just need to be able to open TIFF images, shouldn't PIL be enough? And if the app is meant to become geo aware then I guess a plan needs to laid out to see what to do with the geo metadata that comes with the GeoTIFF.
I don't think we need the images as geo-aware, and you are correct...GDAL, xarray, rasterio are large and somewhat unstable packages that might be good to avoid. What we do want to do is read more bands...more than four. (As many as 12?). I will experiment with reading TIFFs with PIL...the key question is whether it can read >4 bands. |
There's also a library called tifffile which I've never used but that could be an alternative to PIL if it doesn't handle the kind of image you expect the app to support. |
I have confirmed that PIL cannot read my 6-band tif file. I will try tifffile. Thanks! |
My updated |
@csherwood-usgs thanks for the changes using tifffile and your explanations on the follow-up work. I'd suggest that if in the current state of this PR the app works as you expect then we should merge it. As for adapting the core segmentation code, we just need to be careful not to have the code in this repo to diverge from the code in dash-doodler, both apps are meant to use the same code (which will happen when it is put into a package). I would suggest opening an issue in this repo that discusses the changes you want to make for the segmentation to support arrays with more than 3 bands, pinging Dan and others interested in this discussion. |
Superseded by #35 |
Merging from crs_dev. See issue #30