Repository navigation
Easy Importing for the Raster Package - #2891
Conversation
97890a4 to
e090786
Compare
46ebe80 to
1f31224
Compare
There was a problem hiding this comment.
I left a bunch of comments. Issues summary:
- bad rebase
AnyRefmixin 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.
|
@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 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. |
|
@jbouffard Right, this is a good point. But you could leave the generic instance as it is, and to // 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 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 |
|
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 |
|
@metasim it should not be non breaking :D 3.0 would break the API. Btw renaming imports into the similar to |
c066765 to
6639f44
Compare
There was a problem hiding this comment.
@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?
…mport in the vector package Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
…to Implicits.scala Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
…asters Signed-off-by: Author <[email protected]>
…imported into geotrellis.raster. They will now be brought in via the crop.Implicits trait Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
…nto geotrellis.raster. They will now be brought in via the costdistance.Implicits trait Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
…are imported into geotrellis.raster. They will now be brought in via the equalize.Implicits trait Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
…geotrellis.raster. They will now be brought in via the hydrology.Implicits trait Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
… 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 <[email protected]> Signed-off-by: Author <[email protected]>
…are imported into geotrellis.raster. They will now be brought in via the matching.Implicits trait Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Author <[email protected]>
Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Author <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]> Signed-off-by: Author <[email protected]>
…rk package Signed-off-by: Jacob Bouffard <[email protected]> Signed-off-by: Author <[email protected]> Signed-off-by: Author <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
6639f44 to
f8ddcf7
Compare
Signed-off-by: Jacob Bouffard <[email protected]>
Signed-off-by: Jacob Bouffard <[email protected]>
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.
pomadchin
left a comment
pomadchin
left a comment
There was a problem hiding this comment.
LGTM; can we squash commits on merge and add headers into new files?
|
Commits can be squahsed on merge ... files without headers need to be fixed now. I'll push something up. |
4 participants
Overview
This PR simplifies the imports needed to use certain functionality in the
geotrellis.rasterpackage. Now, all implicit methods will be available from thegeotrellis.raster._import. Various types have also been added at therasterpackage level as well.This PR is based on another, open PR: #2885
Checklist
docs/CHANGELOG.rstupdated, if necessarydocsguides update, if necessary