-
Notifications
You must be signed in to change notification settings - Fork 364
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
GeoTrellisRasterSource should return None on empty reads #3240
Conversation
9e78875
to
3e791a9
Compare
tiles: Seq[(SpatialKey, MultibandTile)] with Metadata[TileLayerMetadata[SpatialKey]], | ||
extent: Extent | ||
): Option[Raster[MultibandTile]] = { | ||
val md = tiles.metadata |
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.
There was no tiles.isEmpty
check, and anyway we don't need this function anymore, see #2903
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.
Can you comment a bit further on why we don't need this anymore? That linked issue + its links don't show where the original problem handling missing spatialkeys
is addressed in the original sparsestitch function.
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.
@CloudNiner if you will look a bit above (at the place where it was used):
sparseStitch(tiles, extent)
we can use the inbuilt function now tiles.sparseStitch(extent)
The issue #2903 was addressed in your PR here #3017 which is called "Add sparse stitch method to StitchCollectionMethods".
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.
lol embarrassing -- I do in fact remember this now. 👍
tiles: Seq[(SpatialKey, MultibandTile)] with Metadata[TileLayerMetadata[SpatialKey]], | ||
extent: Extent | ||
): Option[Raster[MultibandTile]] = { | ||
val md = tiles.metadata |
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.
Can you comment a bit further on why we don't need this anymore? That linked issue + its links don't show where the original problem handling missing spatialkeys
is addressed in the original sparsestitch function.
f186d03
to
c5635bd
Compare
c5635bd
to
42c94b4
Compare
Overview
This PR removes a duplicate
GeoTrellisRasterSource.sparseStitch
function that also was not safe enough and could work (incorrectly) with empty inputs.Checklist
Notes
This bug was revelaed during this issue investigation.