-
Notifications
You must be signed in to change notification settings - Fork 361
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
All traits that inherit CellGrid and Grid should be abstract classes #3136
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow... that's it?
Did you confirm that new Raster(tile, extent)
works in Java?
@metasim yep: raster > run
[info] Running RasterInstantiation
[success] Total time: 3 s, completed Oct 22, 2019 12:27:02 PM |
I wonder if the following should preemptively converted to
|
@metasim should be (was in progress) |
Got some more feedback: I think this should be the definition for trait ProjectedRasterLike { _: CellGrid[Int] =>
def crs: CRS
def extent: Extent
} |
@metasim do you want it to be a mixin? |
In RF it's used as a means to show conformity among a few different types for operations that want, say just an Not sure if that qualifies as a mix-in, or just "shape compliance" like an old-style |
@metasim sweet, thanks for looking into it that hard 💯 |
@metasim do we even need this constrain? |
Yes, we need the constraint, because it forces |
Maybe this instead? trait ProjectedRasterLike {
def crs: CRS
def extent: Extent
def cellType: CellType
def cols: Int
def rows: Int
} |
@metasim will look into what is possible to do here |
My vote is for trait ProjectedRasterLike {
def crs: CRS
def extent: Extent
def cellType: CellType
def cols: Int
def rows: Int
} ... version. Trait self typing is another of those weird scala features that isn't really necessary (?). Also proposed variation is more flexible since it doesn't mention |
@echeipesh yep, my point was that we don't need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, solves the problem. Wish we could just remove N
from CellGrid but that is a tricky thing to do with time we have right now.
|
requesting the last quick look at the |
…ow we would like to expose more similar functions
1396f94
to
d35bdf9
Compare
* Fix StreamingByteReader over-allocation past EOF * Add util project to CI tests
@pomadchin this looks good to me. Need changelog entry and version bump to 3.1.0 |
…ow we would like to expose more similar functions
…into fix/tile-hierarchy
Overview
This PR makes Tile, ArrayTile and MultibandTile abstract classes. In our types hierarchy
Tile
types extendCellGrid
andGrid
. In 2.x bothTile
andCellGrid
weretraits
, and in3.x
CellGrid
became an abstract class, so we had a not very usual case, wheretraits
extended abstractclasses
. This is not possible in Java and creates problems on Spark, for more details looks into #3134This is the idea suggested by @echeipesh and it is probably the best solution.
Checklist
Demo
Closes #3134