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

Add split by cols/rows to SplitMethods #1538

Merged
merged 4 commits into from
Jun 13, 2016

Conversation

echeipesh
Copy link
Contributor

This PR adds split(cols: Int, rows: Int) overload to split methods. Existing methods required an input of TileLayout. In order to be able to perform this operation we impose the restriction that SplitMethods must operate on Grid types.

Grid type is new, it is super-class of CellGrid. Specifically we want to be able to split RasterExtent which does not have an associated CellTypes.

Now that type Grid exists there is an outstanding question of what interfaces should be loosened from CellGrid to Grid. This PR does not answer that question.

@@ -64,7 +64,7 @@ case class Raster[+T <: CellGrid](tile: T, extent: Extent) extends Product2[T, E

def cols: Int = tile.cols
def rows: Int = tile.rows
def dimensions: (Int, Int) = tile.dimensions
Copy link
Member

Choose a reason for hiding this comment

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

Why get rid of dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You get those from CellGrid, no need to overwrite here.

@echeipesh
Copy link
Contributor Author

@lossyrob: I thought about your comments on GridDefinition and realized that GridDefinition was just a trait for GridExtent. Merging those two and making the GridExtent -> RasterExtent hierarchy more explicit makes that code less confusing. There is still weird forward reference to RasterExtent with toAllignedRasterExtent method, but I think it's acceptable.

new GridExtent(extent, cellSize.width, cellSize.height)

def apply(extent: Extent, cellwidth: Double, cellheight: Double): GridExtent =
new GridExtent(extent, cellwidth, cellheight)
Copy link
Member

Choose a reason for hiding this comment

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

We lose an unapply here by taking away it's case classy-ness. Also proper equality. I get that you want no case class for inheritance - we should provide that functionality ourselves.

@lossyrob
Copy link
Member

Not super psyched about changing the API after the "API freezing" 0.10 release, but this is a small change and a good change, and a good one to rip the scratch the new floors with. +1 after the case class functionality reimplementation.

@lossyrob lossyrob merged commit 8c0d614 into locationtech:master Jun 13, 2016
@lossyrob lossyrob added this to the 1.0 milestone Oct 18, 2016
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.

2 participants