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

Refactor ResampleGrid names and type parameter #3121

Merged
merged 4 commits into from Oct 11, 2019

Conversation

@moradology
Copy link
Contributor

commented Oct 9, 2019

Overview

We want to avoid type parameters from being propagated throughout the library when they aren't absolutely necessary. This PR hides the [N: Integral] type parameter which was formerly on
each ResampleGrid class. Additionally, it renames ResampleGrid to ResampleTarget (as the grid is what is produced rather than the case class itself) and renames Dimensions as TargetDimensions for consistency and renames the IdentityResampleGrid as DefaultTarget to better reflect its context-dependent nature (the exact operation which is "default" is dependent on which resample/reprojection function is being used.

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • docs guides update, if necessary
  • New user API has useful Scaladoc strings
moradology added 2 commits Oct 9, 2019
We want to avoid type parameters from being propagated throughout
the library when they aren't absolutely necessary. This commit
hides the [N: Integral] type parameter which was formerly on
each ResampleGrid class
@moradology moradology requested a review from pomadchin Oct 9, 2019
*
* @note If used as target of resample operation it acts as identity operation.
*/
case object DefaultResampleTarget extends ResampleTarget {

This comment has been minimized.

Copy link
@echeipesh

echeipesh Oct 9, 2019

Contributor

DefaultResampleTarget is a bit of a mouthful and none of the other subclasses of ResampleTarget have Resample in them. Might I suggest DefaultTarget as alternative ?

This comment has been minimized.

Copy link
@pomadchin

pomadchin Oct 9, 2019

Member

DefaultTarget sounds much better, but the word Target still sounds not that good as "Grid".

Copy link
Member

left a comment

All changes make sense, perfect that there is no type parameter anymore + very useful comments were added to describe ResampleTarget AST.

However the word ResampleTarget, in particular Target here sounds a bit generic for me (like Data in classes names). I don't have better suggestions though.

@@ -62,7 +62,7 @@ class GDALWarpOptionsSpec extends FunSpec with RasterMatchers with GivenWhenThen
rasterExtent = GridExtent(Extent(630000.0, 215000.0, 645000.0, 228500.0), 10, 10),
CRS.fromString("+proj=lcc +lat_1=36.16666666666666 +lat_2=34.33333333333334 +lat_0=33.75 +lon_0=-79 +x_0=609601.22 +y_0=0 +datum=NAD83 +units=m +no_defs "),
WebMercator,
TargetCellSize[Long](CellSize(10, 10))
TargetCellSize(CellSize(10, 10))

This comment has been minimized.

Copy link
@pomadchin

pomadchin Oct 9, 2019

Member

Sweet change!

*
* @note If used as target of resample operation it acts as identity operation.
*/
case object DefaultResampleTarget extends ResampleTarget {

This comment has been minimized.

Copy link
@pomadchin

pomadchin Oct 9, 2019

Member

DefaultTarget sounds much better, but the word Target still sounds not that good as "Grid".

import spire.implicits._

/** Represents a strategy/target for resampling */
sealed trait ResampleTarget {

This comment has been minimized.

Copy link
@pomadchin

pomadchin Oct 9, 2019

Member

Mb these are my language problems, but ResampleGird is a bit more understandable for me rather than ResampleTarget. Is not Target a too generic thing? Like Data when we were designing Paths / DataPaths for RasterSources.

I am not defending ResampleGrid name, I am not fan of it either.

This comment has been minimized.

Copy link
@moradology

moradology Oct 9, 2019

Author Contributor

Target might be too generic, but I guess i'm still a bit stuck on the fact that it isn't a ResampleGrid so much as a strategy/target by which a grid will be summoned/constructed

This comment has been minimized.

Copy link
@pomadchin

pomadchin Oct 9, 2019

Member

@moradology totally agree, it is definitely not a ResampleGrid, I don't have a better naming suggestion, so mb it is fine as you did.

@pomadchin

This comment has been minimized.

Copy link
Member

commented Oct 9, 2019

LGTM in general, there are only minor naming issues and it can be merged once all comments are answered and CI is happy.

@moradology moradology force-pushed the moradology:feature/resample-target branch from 6ffd48d to ad2c2f4 Oct 10, 2019
@echeipesh echeipesh merged commit ac92a45 into locationtech:master Oct 11, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
echeipesh added a commit to pomadchin/geotrellis that referenced this pull request Oct 15, 2019
* Rename ResampleGrid; hide its type param

We want to avoid type parameters from being propagated throughout
the library when they aren't absolutely necessary. This commit
hides the [N: Integral] type parameter which was formerly on
each ResampleGrid class

* Rename DefaultResampleTarget to DefaultTarget

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