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.

Sign up for GitHub

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

Jump to bottom

Reprojection Short Circuting #209

Merged
merged 1 commit into from
Jul 17, 2019
Merged

Reprojection Short Circuting #209

merged 1 commit into from
Jul 17, 2019

Conversation

jbouffard
Copy link
Contributor

@jbouffard jbouffard commented Jul 11, 2019 •
edited
Loading

Overview

This PR adds logic that checks to see if the target RasterSource need to be reprojected. If they don't, then they are either resampled are just returned as-is.

Checklist

  • Add entry to CHANGELOG.md

Closes #102

@moradology moradology self-assigned this Jul 15, 2019
metasim
metasim reviewed Jul 15, 2019
View reviewed changes
vlm/src/main/scala/geotrellis/contrib/vlm/RasterSource.scala
/** Reproject to different CRS with explicit sampling reprojectOptions.
* @see [[geotrellis.raster.reproject.Reproject]]
* @group reproject
*/
def reproject(crs: CRS, reprojectOptions: Reproject.Options, strategy: OverviewStrategy): RasterSource
def reproject(crs: CRS, reprojectOptions: Reproject.Options, strategy: OverviewStrategy): RasterSource =
if (crs == this.crs) this
Copy link
Contributor

Choose a reason for hiding this comment

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

Be aware that this can be very computationally expensive. See locationtech/geotrellis#3022

Copy link
Contributor

Choose a reason for hiding this comment

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

See also: locationtech/rasterframes#134

Copy link
Member

Choose a reason for hiding this comment

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

@metasim #208

moradology
moradology requested changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@moradology moradology left a comment

Choose a reason for hiding this comment

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

The PR looks good overall, but I'm not comfortable OKing the use of internal for methods expected of implementers. This deserves a bit more thought

@moradology
Copy link
Contributor

moradology commented Jul 16, 2019 •
edited
Loading

I've thought about this a bit and I've got a potential solution which may be controversial. Instead of reproject being the externally facing method, we could find an idempotent way of phrasing things (short circuiting ensures idempotence). Thus, we might have:

val rs: RasterSource = GeotiffRasterSource(...)
rs.projected(crs, reprojectOptions, strategy)

Our expectation via the trait, on the other hand, would be a method named reproject which only gets called if the projection isn't already the one requested

@jbouffard
Copy link
Contributor Author

@moradology @echeipesh @pomadchin have we decided on a naming strategy for the externally facing method?

@moradology
Copy link
Contributor

Another option brought up by @pomadchin which I think deserves our attention and would be simpler for now is to use reproject as we currently are and change reprojectInternal to reprojection

@jbouffard
Copy link
Contributor Author

I think they're both good options. @pomadchin's might be better in the short term, as it doesn't require a refactor, but I think we should implement something similar to what @moradology suggested later on.

@echeipesh
Copy link
Collaborator

@jbouffard I think thats a reasonable compromise to close out this PR.

@jbouffard / @moradology
Given that the iron is hot the issue for the the idempotent reprojection should include the mockup of effected methods and related. I continue to like the idea and I think it is worth further thought.

@moradology moradology mentioned this pull request Jul 16, 2019
Open
Improved the reprojected logic so that it now checks to see if a repr…
dc58f04 
…ojection can be skipped

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Updated the CHANGELOG

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Updated remaining reproject method

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Fixed failing test

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Renamed _reproject to reprojectInternal

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Fixed spelling

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

Renamed the reprojectInternal method to reprojection

Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
pomadchin
pomadchin approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pomadchin pomadchin merged commit 9ae49ae into geotrellis:master Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@metasim metasim metasim left review comments

@echeipesh echeipesh echeipesh left review comments

@moradology moradology moradology requested changes

@pomadchin pomadchin pomadchin approved these changes

Assignees

@moradology moradology

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RasterSource.reproject to CRS of underlying file should short-circuit
5 participants
@jbouffard @moradology @echeipesh @metasim @pomadchin