Skip to content

Filter GeoTiffRDDs by Geometry - #2409

Merged
echeipesh merged 4 commits into
locationtech:masterfrom
jamesmcclain:feature/geotiff-filtering
Oct 13, 2017
Merged

Filter GeoTiffRDDs by Geometry#2409
echeipesh merged 4 commits into
locationtech:masterfrom
jamesmcclain:feature/geotiff-filtering

Conversation

@jamesmcclain

@jamesmcclain jamesmcclain commented Oct 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

pomadchin
pomadchin reviewed Oct 10, 2017
View reviewed changes
Comment thread s3/src/main/scala/geotrellis/spark/io/s3/S3GeoTiffRDD.scala Outdated
bucket: String, prefix: String,
uriToKey: (URI, I) => K,
options: Options,
geometry: Option[Geometry] = None

@pomadchin pomadchin Oct 10, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, mb to add some helpful overloads? Though it's smth to discuss, as the best otpion would be to move it into Options and it's impossible due to compatibility reasons. ):

@echeipesh echeipesh Oct 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There also needs to be an apply with the old signature to avoid removing this method from the API.

pomadchin
pomadchin approved these changes Oct 10, 2017
View reviewed changes

@pomadchin pomadchin left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mb in 2.0 we'll move geometry into Options but for now it's good. Looks like it can be merged as is after #2402

Review TIP

@pomadchin

pomadchin commented Oct 10, 2017

Copy link
Copy Markdown
Member

Btw a thought appeared after the review: mb it makes sense to add some extra overloads to use this filter by geometry functionality more convenient.

@jamesmcclain

jamesmcclain commented Oct 11, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Btw a thought appeared after the review: mb it makes sense to add some extra overloads to use this filter by geometry functionality more convenient.

Yes, perhaps we can do that in a subsequent PR (or not, depending on timing).

@jamesmcclain
jamesmcclain force-pushed the feature/geotiff-filtering branch from 4fb92b2 to bbd21aa Compare October 11, 2017 16:37
@rossbernet rossbernet added this to the 1.2 milestone Oct 11, 2017
@jamesmcclain
jamesmcclain force-pushed the feature/geotiff-filtering branch from bbd21aa to 10ec91f Compare October 11, 2017 19:42
echeipesh
echeipesh requested changes Oct 12, 2017
View reviewed changes
Comment thread s3/src/main/scala/geotrellis/spark/io/s3/S3GeoTiffRDD.scala Outdated
bucket: String, prefix: String,
uriToKey: (URI, I) => K,
options: Options,
geometry: Option[Geometry] = None

@echeipesh echeipesh Oct 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There also needs to be an apply with the old signature to avoid removing this method from the API.

@jamesmcclain
jamesmcclain force-pushed the feature/geotiff-filtering branch from 10ec91f to c33e89b Compare October 12, 2017 13:39
@echeipesh
echeipesh merged commit e88c094 into locationtech:master Oct 13, 2017
@fosskers fosskers mentioned this pull request Oct 13, 2017
Closed
@jamesmcclain
jamesmcclain deleted the feature/geotiff-filtering branch November 17, 2017 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@echeipesh echeipesh echeipesh approved these changes
@pomadchin pomadchin pomadchin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.2

Development

Successfully merging this pull request may close these issues.

5 participants

@jamesmcclain @pomadchin @echeipesh @fosskers @rossbernet