Don't constrain GridBounds size to IntMax x IntMax by fosskers · Pull Request #2292 · locationtech/geotrellis

fosskers · 2017-07-20T20:36:59Z

Issues

size returning a Long instead of Int is likely backwards incompatible.
I wanted to make coords return an Iterator instead of a Stream, but one single function (RDDKernelDensity.pointFeatureToSpatialKey) in GT requires the output of a for over coords
to be a Seq, which Array can be cast to and Iterator cannot.
I ingested a 60k-by-60k GeoTiff after making this change, and while the ingest itself was very strange (took forever to process, was all single core, no I didn't mess up the Spark config), it did succeed.

Closes #2265 .

fosskers · 2017-07-20T20:42:18Z

+  def coords: Stream[(Int, Int)] = for {
+    row <- Stream.range(0, height)
+    col <- Stream.range(0, width)
+  } yield (row, col)


I'd really love to salvage the ability to use Iterator here.

lossyrob · 2017-07-20T20:46:13Z

The problem here of course is that this breaks the API. Such a minor break, but still breaks it.
If we were to release this as a bugfix release, because it's fixing a bug, the semver gods will frown down upon us.

Client code like:

val gb: GridBounds = ???
val gridSize = gb.size

would be fine, but code like

val gb: GridBounds = ???
val gridSize:Int = gb.size

would fail.

It might be the case that exactly 0 codebases have code that would break on this change. But then again, maybe a million codebases. Or like 2.

Now that we switched to semver how do we handle this? Any thoughts from others? @hectcastro? @echeipesh? @metasim?

fosskers · 2017-07-20T20:48:36Z

Regarding appeasing the SemVer gods, I suppose we'd have to wait to merge until we're ready to push more 2.0 features through?

fosskers · 2017-07-20T20:56:13Z

The Array => Stream change is also breaking. They're both collections, but they don't support exactly the same operations. GridBounds.coords is never used this way in GT itself, but technically there was nothing stopping users from doing:

val gbs: GridBounds = ???
val coords: Array[(Int, Int)] = dbs.coords

println(coords(5)) // caution to the wind. Also a senseless operation.

Open question: do we have a right to overlook API breaks when we have sufficient belief that a certain use case (like this one) would never occur in reality? More of me wants to say "no" than "yes", but the "yes" is there nonetheless.

echeipesh · 2017-07-20T21:24:36Z

GridBounds floats to the surface of the API way too often for us to pretend that its somehow a private class. I don't think I've ever written even a small module that didn't and up holding GridBounds in some way.

I'd be tempted to add: .sizeLong: Long and .cordsIterator: Iterator[(Int, Int)] and try to switch the breaking code to use the new methods. This could spider out but then again it could be tidy.

A cleaner version of above is to make a new class LongGridBounds or some such and convert to it to perform the above operations.

fosskers · 2017-07-20T21:31:59Z

... to pretend that its somehow a private class.

Luckily I wasn't suggesting that. Can you think of a reason someone would honestly index the coords Array as in my example?

Alarm bells going off regarding LongGridBounds. The bug is occurring in the original class as used in vanilla ingests, so I don't think LGB would solve that.

The .*Long varieties might work, but they birth another hidden bug: the original coords Array being too big and throwing. Okay, so we bring in the invariant back in to protect against that... and we're back to square one.

echeipesh · 2017-07-20T21:36:05Z

Luckily I wasn't suggesting that. Can you think of a reason someone would honestly index the coords Array as in my example?

I didn't not mean that as an accusation, just not an option.

I would disagree that original .coords throwing when its too big is a hidden bug, thats pretty much a standard data-structure choice of sequence vs. an iterator. And we're free to @depricate it with a meaningful warning.

Edit: And I agree there is totally no reason to index into array of coords, the discussion is purely around being good software carpenters as prescribed by semver.

fosskers · 2017-07-20T21:38:31Z

thats pretty much a standard data-structure choice of sequence vs. an iterator.

Yeah true. So if it blows, we say "told you so, here's the Iterator variety for that". @deprecated feels right.

The Stream/Iterator issue is still outstanding though.

fosskers · 2017-07-20T23:27:01Z

-  def size = width * height
-  def isEmpty = size == 0
+
+  def size: Int = width * height


I tried to use the @migration annotation which is mentioned in the official Scaladoc docs, but it doesn't compile. There's no mention of it in the main scala repo.

fosskers · 2017-07-20T23:29:00Z

        val extent = inKey.extent
        logger.debug(s"Cutting $inKey of ${tile.dimensions} cells covering $extent")
        mapTransform(extent)
-          .coords.toIterator


- The original `coords` has been marked deprecated, and will be switched with `coordsIter` in 2.0.

hectcastro · 2017-07-21T12:29:46Z

I agree with Eugene's instincts around sizeLong and LongGridBounds. It would be great if we could come up with replacement function or class names that we'd want to keep moving forward (e.g., coords -> coordinates).

size seems a lot trickier. I wasn't able to come up with a good alternative name that didn't lead to confusion. In this case, we may have no choice but to go with something like sizeLong, mark size for deprecation (but note that its type will change vs. actual function removal), rewrite size to return type Long in 2.0, and mark sizeLong for deprecation in 3.0.

As far as the high level plan moving forward, I think the process would be:

Publish the new functions with deprecation warnings on the old functions in a minor release
Highlight the above change in the CHANGELOG, or anywhere else the 1.2 release Is discussed
Schedule removal of the deprecated functions in a major release

fosskers · 2017-07-21T14:17:56Z

I can add a deprecation warning to size (already done so for sizeLong), and update the CHANGELOG to include these warnings.

fosskers · 2017-07-21T19:41:26Z

Figuring out test failures...

fosskers mentioned this pull request Jul 20, 2017

Cannot Create Gridbounds for Layer Ingest #2265

Closed

fosskers commented Jul 20, 2017

View reviewed changes

fosskers changed the title ~~Don't constrain GridBounds size to IntMax x IntMax~~ [WIP] Don't constrain GridBounds size to IntMax x IntMax Jul 20, 2017

Don't constrain GridBounds size to IntMax x IntMax

2c2c3cd

fosskers force-pushed the fix/cgw/big-gridbounds branch from 1a886a0 to 49e5f63 Compare July 20, 2017 23:25

fosskers commented Jul 20, 2017

View reviewed changes

Provide GridBounds.coordsIter and use it where possible

2ecceff

- The original `coords` has been marked deprecated, and will be switched with `coordsIter` in 2.0.

fosskers force-pushed the fix/cgw/big-gridbounds branch from 49e5f63 to 2ecceff Compare July 21, 2017 00:11

Update CHANGELOG, provide proper deprecation annotations

d93600e

fosskers changed the title ~~[WIP] Don't constrain GridBounds size to IntMax x IntMax~~ Don't constrain GridBounds size to IntMax x IntMax Jul 21, 2017

GridBounds: coordsIter output should match coords

d1b9b39

fosskers force-pushed the fix/cgw/big-gridbounds branch from 3d987f2 to d1b9b39 Compare July 21, 2017 21:50

echeipesh approved these changes Jul 25, 2017

View reviewed changes

echeipesh added this to the 1.2 milestone Jul 25, 2017

echeipesh merged commit c9db248 into master Jul 25, 2017

fosskers mentioned this pull request Jul 25, 2017

HadoopGeoTiffRDD (at least) reads segments multiple times #2299

Closed

fosskers deleted the fix/cgw/big-gridbounds branch August 17, 2017 16:46

Navigation Menu

Repository navigation

Don't constrain GridBounds size to IntMax x IntMax - #2292

Don't constrain GridBounds size to IntMax x IntMax#2292
echeipesh merged 4 commits into
masterfrom
fix/cgw/big-gridbounds

fosskers commented Jul 20, 2017 •

edited

Loading

Uh oh!

fosskers Jul 20, 2017

Uh oh!

lossyrob commented Jul 20, 2017

Uh oh!

fosskers commented Jul 20, 2017

Uh oh!

fosskers commented Jul 20, 2017 •

edited

Loading

Uh oh!

echeipesh commented Jul 20, 2017

Uh oh!

fosskers commented Jul 20, 2017

Uh oh!

echeipesh commented Jul 20, 2017 •

edited

Loading

Uh oh!

fosskers commented Jul 20, 2017 •

edited

Loading

Uh oh!

fosskers Jul 20, 2017

Uh oh!

fosskers Jul 20, 2017

Uh oh!

hectcastro commented Jul 21, 2017

Uh oh!

fosskers commented Jul 21, 2017

Uh oh!

fosskers commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

fosskers commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issues

Uh oh!

fosskers Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

lossyrob commented Jul 20, 2017

Uh oh!

fosskers commented Jul 20, 2017

Uh oh!

fosskers commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

echeipesh commented Jul 20, 2017

Uh oh!

fosskers commented Jul 20, 2017

Uh oh!

echeipesh commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fosskers commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fosskers Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

fosskers Jul 20, 2017

Choose a reason for hiding this comment

Uh oh!

hectcastro commented Jul 21, 2017

Uh oh!

fosskers commented Jul 21, 2017

Uh oh!

fosskers commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fosskers commented Jul 20, 2017 •

edited

Loading

fosskers commented Jul 20, 2017 •

edited

Loading

echeipesh commented Jul 20, 2017 •

edited

Loading

fosskers commented Jul 20, 2017 •

edited

Loading