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

Easy Importing for the Raster Package #2891

Merged
merged 67 commits into from
May 24, 2019

Conversation

jbouffard
Copy link
Contributor

@jbouffard jbouffard commented Apr 3, 2019

Overview

This PR simplifies the imports needed to use certain functionality in the geotrellis.raster package. Now, all implicit methods will be available from the geotrellis.raster._ import. Various types have also been added at the raster package level as well.

This PR is based on another, open PR: #2885

Checklist

  • docs/CHANGELOG.rst updated, if necessary
  • docs guides update, if necessary

@jbouffard jbouffard force-pushed the feature/easy-imports/raster branch from 97890a4 to e090786 Compare April 3, 2019 15:20
@jbouffard jbouffard force-pushed the feature/easy-imports/raster branch from 46ebe80 to 1f31224 Compare April 25, 2019 16:06
@jbouffard jbouffard changed the base branch from refactor/terse-imports to master April 25, 2019 16:07
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.

I left a bunch of comments. Issues summary:

  • bad rebase
  • AnyRef mixin for some reason into some implicit classes
  • no changelog update
  • this thing below:

I'm also worrying about non consistent changes, I am wondering, why the following classes became less abstract: TileFeatureCropMethods, RasterTileFeatureCropMethods, RasterTileFeatureMaskMethods. For some reason some type classes were made less abstract and some extra boilerplate was introduced. It was done inconsistently so I could even miss smth.

Also some unnecessary aliases were added: for instance:

abstract class TileRasterCropMethods extends RasterCropMethods[Tile]

I am not sure that it makes things easier or more transparent, it looks like it just introduces more classes into the class hierarchy. Tbh in terms of this PR I think all of them can be just deleted as such classes don't simplify code at all, but make it harder to read.

I can accept the usecase of such abstract classes necessity in cases where you have 3 or 4 type parameters and you heavily reuse it, but I'm afraid it is not about this PR.

@jbouffard
Copy link
Contributor Author

@pomadchin The reason that the classes have become less abstract is because that was preventing easy workflow in the console.

As an example, I'm using the current master to try and crop a Raster[Tile], but I need to mark that value as an Raster[Tile] explicitly or else it won't work.

scala> import geotrellis.raster._
import geotrellis.raster.resample._
import geotrellis.vector._
import geotrellis.raster.io.geotiff._
import geotrellis.raster.render._


scala> val tile = IntArrayTile(0 until 15 toArray, 4, 4)
tile: geotrellis.raster.IntArrayTile = IntConstantNoDataArrayTile([I@46f3887e,4,4)

scala> val ex = Extent(0, 0, 10, 10)
ex: geotrellis.vector.Extent = Extent(0.0, 0.0, 10.0, 10.0)

scala> val raster = Raster(tile, ex)
raster: geotrellis.raster.Raster[geotrellis.raster.IntArrayTile] = Raster(IntConstantNoDataArrayTile([I@46f3887e,4,4),Extent(0.0, 0.0, 10.0, 10.0))

scala> raster.
_1          canEqual   cols   dimensions   geom                    gridBounds   mapGeom        productElement    rasterExtent   rows   tile       
_2          cellSize   copy   equals       getDoubleValueAtPoint   hashCode     mapTile        productIterator   reproject      self   toString   
asFeature   cellType   data   extent       getValueAtPoint         mapData      productArity   productPrefix     resample       size   toVector   

scala> val raster: Raster[Tile] = Raster(tile, ex)
raster: geotrellis.raster.Raster[geotrellis.raster.Tile] = Raster(IntConstantNoDataArrayTile([I@46f3887e,4,4),Extent(0.0, 0.0, 10.0, 10.0))

scala> raster.
_1          cellSize   crop         extent           getDoubleValueAtPoint   mapData   merge             productPrefix   rotate180   rows    tile       
_2          cellType   data         flipHorizontal   getValueAtPoint         mapGeom   productArity      rasterExtent    rotate270   self    toString   
asFeature   cols       dimensions   flipVertical     gridBounds              mapTile   productElement    reproject       rotate360   size    toVector   
canEqual    copy       equals       geom             hashCode                mask      productIterator   resample        rotate90    split              

scala> raster.

By making things less abstract, we'll be able to access methods on these values without having to worry about assigning them the correct type.

@pomadchin
Copy link
Member

pomadchin commented Apr 26, 2019

@jbouffard Right, this is a good point. But you could leave the generic instance as it is, and to extend it making avail with less implicits in the call scope:

// this is the old one class
abstract class TileFeatureCropMethods[
  T <: CellGrid[Int] : (? => TileCropMethods[T]),
  D
](val self: TileFeature[T, D]) extends TileCropMethods[TileFeature[T, D]] {
  import Crop.Options

  def crop(srcExtent: Extent, extent: Extent, options: Options): TileFeature[T, D] =
    TileFeature(self.tile.crop(srcExtent, extent, options), self.data)

  def crop(gridBounds: GridBounds[Int], options: Options): TileFeature[T, D] =
    TileFeature(self.tile.crop(gridBounds, options), self.data)
}

abstract class SinglebandTileFeatureCropMethods[D](self: TileFeature[Tile, D]) extends TileFeatureCropMethods[Tile, D](self)
abstract class MultibandTileFeatureCropMethods[D](self: TileFeature[MultibandTile, D]) extends TileFeatureCropMethods[MultibandTile, D](self)

To compile these specific instances, compiler already would derive all implicits, so if your implicit classes would extend {Singleband, Multiband}TileFeatureCropMethods[D] no extra imports would be required to use it.

Also such approach will allow you to get rid of that extra hand-written boilerplate you generated.

This is basically useful probably for all future imports simplifications; we don't really need to get rid of these good and high abstract classes, but we can simplify their usage.

At this point this PR brings in 742 new potentially boilerplate lines ):

@metasim
Copy link
Member

metasim commented Apr 28, 2019

There are a lot of changes here that I worry could be breaking, yet hard to document a migration path. I can't point to anything specific, just gut paranoia. I would want to see if it breaks the RasterFrames build.

Another approach to consider is a separate, new package akin to cats.syntax.arrow vs. cats.syntax.all, whereby import semantics are defined by the different packages, and people can opt-in to the new import semantics, but not break the code of legacy users.

@pomadchin
Copy link
Member

pomadchin commented Apr 28, 2019

@metasim it should not be non breaking :D 3.0 would break the API.
However, talking about these imports - nothing really would change.

Btw renaming imports into the similar to cats structure sounds like a different and a separate task.

@pomadchin pomadchin mentioned this pull request May 2, 2019
2 tasks
@pomadchin pomadchin added this to the 3.0 milestone May 2, 2019
@jbouffard jbouffard force-pushed the feature/easy-imports/raster branch from c066765 to 6639f44 Compare May 2, 2019 19:15
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.

@jbouffard can you rebase on top of master and squash commits a little bit by some logical pieces, during the iterative rebase? Also, is the checklist done?

jbouffard added 15 commits May 21, 2019 09:27
…mport in the vector package

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

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

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

Signed-off-by: Author <jbouffard@azavea.com>
…to Implicits.scala

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

Signed-off-by: Author <jbouffard@azavea.com>
…asters

Signed-off-by: Author <jbouffard@azavea.com>
…imported into geotrellis.raster. They will now be brought in via the crop.Implicits trait

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

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

Signed-off-by: Author <jbouffard@azavea.com>
…nto geotrellis.raster. They will now be brought in via the costdistance.Implicits trait

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

Signed-off-by: Author <jbouffard@azavea.com>
…are imported into geotrellis.raster. They will now be brought in via the equalize.Implicits trait

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

Signed-off-by: Author <jbouffard@azavea.com>
…geotrellis.raster. They will now be brought in via the hydrology.Implicits trait

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

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

Signed-off-by: Author <jbouffard@azavea.com>
… imported into geotrellis.raster. They will now be brought in via the focal.Implicits, hillshade.Implicits, local.Implicits, and zonal.Implicits traits

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

Signed-off-by: Author <jbouffard@azavea.com>
…are imported into geotrellis.raster. They will now be brought in via the matching.Implicits trait

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

Signed-off-by: Author <jbouffard@azavea.com>
Signed-off-by: Author <jbouffard@azavea.com>
Signed-off-by: Author <jbouffard@azavea.com>
jbouffard added 16 commits May 21, 2019 09:29
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>

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

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

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

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

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

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

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

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

Signed-off-by: Author <jbouffard@azavea.com>
…rk package

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

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

Signed-off-by: Author <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
@jbouffard jbouffard force-pushed the feature/easy-imports/raster branch from 6639f44 to f8ddcf7 Compare May 21, 2019 14:30
jbouffard and others added 3 commits May 24, 2019 08:29
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
Signed-off-by: Jacob Bouffard <jbouffard@azavea.com>
There is a deeper potential issue here, that `SplitMethods[V]` are not being built up the same way they were before. That should be addressed either in conversion to simulacrum type class encoding or during release testing.
Unclear if its a problem in general, this commit at least cleans up the signature at cost of a little code duplication.
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; can we squash commits on merge and add headers into new files?

@echeipesh
Copy link
Contributor

Commits can be squahsed on merge ... files without headers need to be fixed now. I'll push something up.

@echeipesh echeipesh merged commit 990f146 into locationtech:master May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants