Fix gdal-warp -ovr ports by moradology · Pull Request #3200 · locationtech/geotrellis

moradology · 2020-03-10T16:57:19Z

Overview

This PR exposes the overview selection logic implicit in each of the OverviewStrategy instances by way of a selectOverview method. Now, instead of matching and requiring users of the sum type to implement logic themselves, a call to selectOverview with the available CellSizes and the desired CellSize will yield a selection of the CellSize corresponding to the selected Overview

Checklist

docs/CHANGELOG.rst updated, if necessary
docs guides update, if necessary
New user API has useful Scaladoc strings
Unit tests added for bug-fix or new feature

Notes

Optional. Ancillary topics, caveats, alternative strategies that didn't work out, anything else.

Closes #3196

pomadchin

LGTM and in general it fixes the problem (in case travis is happy). Apart of a small code hygiene things and the lack of a CHANGELOG, my main question is about the def selectOverview function that probably should not be a part of AST, since can be used only in a single known usecase.

Would be happy to talk through it, mb my API concerns are not applicable here.

/cc @moradology @echeipesh // and anyone else if Eugene is busy // we need 3 persons for the consensus? :D

P.S. It looks like some specs are failing i.e. geotrellis.layer.LayoutTileSourceSpec => there is some bug in a new overview selection function. Let's try to be as conservative as it is possible to preserve the old behavior (or some compat) to make RF life easier.

/cc @notthatbreezy & @jisantuc you were using it in RasterFoundry mentioning it here because this can be a pretty sensitive change for RasterFoundry.

pomadchin · 2020-03-10T17:21:12Z

+
+import org.scalatest._
+
+class OverviewStrategySpec extends FunSpec with Matchers {


+1000 for Specs 👍

pomadchin

Once Travis is happy I think we can merge it 👍

pomadchin · 2020-03-11T19:26:04Z

+    case Base =>
+      overviewCS.indexOf(overviewCS.min)
+    case AutoHigherResolution =>
+      selectIndexByProximity(overviewCS, desiredCS, 1.0)


Waiting for Travis to be happy with this change. This behavior should match to the tested old behavior (the old one had a long testing path).

The tests turned out to require a bit more attention than I'd anticipated as some of them baked in the AutoHigher behavior rather than Auto

pomadchin

Once Travis is happy I think we can merge it, but I decided to put a changes request to review it once tests would be fixed 👍; Hope this would not be too bad.

Implementation wasn't updated to properly use OverviewStrategy Also updates OverviewStrategy.selectOverview to clamp returned indices to valid range

pomadchin

LGTM, but have questions about why everywhere we use Base instead of the former default AutoHigher (as a sainity check that we preserved the old behavior).

And is it okay that getClosestResolution changed its signature? (im fine with that btw, I don't have a strong opinion about that function atm). If so, what kind of errors can it throw? Can we add a small test? Before this change it returned an Option type so nothing really was to test here.

pomadchin · 2020-03-31T19:21:13Z

-      case Base => maxResultion
-    }
+    strategy: OverviewStrategy = OverviewStrategy.DEFAULT
+  )(implicit f: T => CellSize): T = {


And this change made this function throw, do we have tests for that? What kind of exceptions can it throw?

Ah, should have mentioned this sooner.

It was Option[Int] because some operations could return an out of bounds index. It's not clear how GDAL handles this situation, so we decided that it would be simpler if OverviewStrategy.selectOverview always returned an in bounds index. Which it now does due to additional error handling there. Before this change, we were doing an option.get everywhere which just pushes the throw further down. In that case, I wasn't sure how we would even handle getting a bad overview.

I can add some additional docstrings to note that selectOverview should always return a clamped index, this was apparently the intent of the code based on a test that predates me picking this task up.

When it can’t find an overview the idea was to use the Base layer, since it looks like what GDAL does. That is why function returned Option - it is a normal behavior not to find any overviews

How does that reconcile with the idea that we've decided to include the base layer in the resolutions list in all RasterSource implementations?

Perhaps this discussion should be moved offline as well?

After a discussion we decided on the following:

Revert OverviewStrategy.DEFAULT = AutoHigherResolution to make the default consistent with the old behavior for now. We'll address changing the default at a later date

Update AutoHigherResolution implementation to match the docstring and old behavior, and verify it with tests

OverviewStrategy.selectOverview still clamps to available indices. Each overview strategy can perform custom clamping behavior, like AutoHigher above, but in the absence of that, the index will be clamped to the nearest valid value, e.g. -1 -> 0 or list.size + 5 -> list.size - 1

Revert tests where possible to use the AutoHigher default in order to appropriately test the old behavior.

Fixes methods in: - RasterSourceRDD.apply - RasterSource.reprojectToGrid where OverviewStrategy was not correctly passed to some resample operations. **NOTE:**: RasterSourceRDD still inconsistently uses both ResampleMethod and OverviewStrategy. For best results, its necessary to use RasterSourceRDD.apply directly.

Same issue as in previous commit for RasterSourceRDDSpec, need to provide strategy. See: locationtech#3216

Sets to WARN by default, to reduce verbosity in build logs which make it difficult to see the test results.

In RasterSourceRDD tests in order to better preserve original test intent (since the default strategy used to be AutoHigher)

To retain backwards compatibility with existing code that uses the default strategy. We expect to change this default to Auto(0) in the future.

Specifically it should now clamp to Base if the index is OOB in any direction

pomadchin · 2020-04-01T23:02:51Z

+  // This method is unsafe. It's up to the selectOverview method to handle each index OOB error for
+  // each indivdual strategy
+  private def selectIndexByProximity(overviewCS: List[CellSize], desiredCS: CellSize, proximityThreshold: Double): Int =
+    overviewCS.search(desiredCS) match {


I guess this could be replaced with a foldM function. Amyway this list should be sorted ¯_(ツ)_/¯ so it could be more easy to read.

pomadchin · 2020-04-02T01:19:19Z

+  /**
+    * Select appropriate overview given the strategy.
+    *
+    * WARN: this function assumes that CellSizes are sorted. It interprets idx 0 as the position with the highest CellSize.


Taking into the account that this is true, usage of the search function in the selectIndexByProximity function makes it really hard to interpret.

pomadchin · 2020-04-02T01:22:00Z

+        val idx = selectIndexByProximity(overviewCS, desiredCS, 1.0)
+        // AutoHigherResolution defaults to the closest level
+        // idx > overviewCS.size means, that idx = overviewCS.size (last idx + 1)
+        if (idx > overviewCS.size) idx - 1 else idx


We need to know where is the highest resolution to interpret the index the selectIndexByProximity function returns. If the input list is sorted, idx > overviewCS.size means the lower resolution. In case it is reversed, it means the higher resolution. If the list is not sorted, this index makes no sense for its interpretation.

… search

pomadchin · 2020-04-02T14:31:39Z

+    * @return                   index of the closest cellSize. Returns -1 in case nothing was found or the input list was not sorted.  
+    */
+  private def selectIndexByProximity(overviewCS: List[CellSize], desiredCS: CellSize, proximityThreshold: Double): Int =
+    overviewCS.zipWithIndex.foldLeftM(Option.empty[(CellSize, Int)]) { case (acc, r @ (rightCZ, _)) =>


I am not sure that with folding it is much easier to read, however it is more explicit about sorting.

pomadchin · 2020-04-02T14:32:00Z

+          // - negative if rightCZ < leftCZ
+          // - positive if rightCZ > leftCZ
+          // - zero otherwise (if rightCZ == leftCZ)
+          if (Ordering[CellSize].compare(rightCZ, leftCZ) < 0) {


short circuiting if the list is not sorted

moradology requested a review from pomadchin March 10, 2020 16:57

pomadchin requested changes Mar 10, 2020

View reviewed changes

moradology force-pushed the fix/gdal-auto-overview-strategy branch from bc6a3c2 to 4f7fe57 Compare March 11, 2020 19:06

moradology requested a review from pomadchin March 11, 2020 19:07

pomadchin approved these changes Mar 11, 2020

View reviewed changes

pomadchin requested changes Mar 11, 2020

View reviewed changes

moradology commented Mar 25, 2020

View reviewed changes

Comment thread layer/src/test/scala/geotrellis/raster/geotiff/GeoTiffReprojectRasterSourceSpec.scala Outdated

pomadchin reviewed Mar 26, 2020

View reviewed changes

Comment thread layer/src/test/scala/geotrellis/raster/geotiff/GeoTiffReprojectRasterSourceSpec.scala Outdated

pomadchin assigned CloudNiner Mar 30, 2020

moradology mentioned this pull request Mar 31, 2020

RasterSourceRDD resample + overview params missing #3216

Open

moradology and others added 5 commits March 31, 2020 15:03

Fix gdal-warp -ovr ports

0504e09

Include base tiff among overviews to select from

46509a5

Fix gdal overview test with bad expectation

a034078

Add tests for Auto as well as AutoHigher

e229882

Fix GTRS.getClosestResolution

4822a4b

Implementation wasn't updated to properly use OverviewStrategy Also updates OverviewStrategy.selectOverview to clamp returned indices to valid range

CloudNiner force-pushed the fix/gdal-auto-overview-strategy branch 2 times, most recently from 47498f0 to ce931ed Compare March 31, 2020 19:15

pomadchin requested changes Mar 31, 2020

View reviewed changes

CloudNiner added 6 commits April 1, 2020 10:13

Disable supershell for sane build logs

d3fd487

Fix broken GDALRasterSourceRDDSpec

d88c12f

Same issue as in previous commit for RasterSourceRDDSpec, need to provide strategy. See: locationtech#3216

Add log4j config for gdal-spark

cb564e7

Sets to WARN by default, to reduce verbosity in build logs which make it difficult to see the test results.

Revert to AutoHigherResolution strategy

155689e

In RasterSourceRDD tests in order to better preserve original test intent (since the default strategy used to be AutoHigher)

Pass strategy through RasterSource.reprojectToRegion

e2ab61e

CloudNiner mentioned this pull request Apr 1, 2020

Add toString and comments #3217

Merged

CloudNiner added 3 commits April 1, 2020 10:21

Revert RasterSourceRDD specs to previous implementation

b4c4ca1

Revert default OverviewStrategy to AutoHigher

5c4c823

To retain backwards compatibility with existing code that uses the default strategy. We expect to change this default to Auto(0) in the future.

Make AutoHigherResolution consistent with docstring

d930be7

Specifically it should now clamp to Base if the index is OOB in any direction

CloudNiner force-pushed the fix/gdal-auto-overview-strategy branch from 96ab3da to d930be7 Compare April 1, 2020 14:42

pomadchin force-pushed the fix/gdal-auto-overview-strategy branch from ebfa9cb to f62214e Compare April 1, 2020 22:09

pomadchin force-pushed the fix/gdal-auto-overview-strategy branch from f62214e to 0bcb552 Compare April 1, 2020 22:12

pomadchin reviewed Apr 1, 2020

View reviewed changes

Remove a new test (we'll add it later); fix selectOverview function

5d7b6fd

pomadchin force-pushed the fix/gdal-auto-overview-strategy branch from 0bcb552 to 5d7b6fd Compare April 1, 2020 23:15

pomadchin reviewed Apr 2, 2020

View reviewed changes

Fix GeoTrellisRasterSources overview selection strategy

ed78d83

pomadchin force-pushed the fix/gdal-auto-overview-strategy branch from 7ffbf11 to ed78d83 Compare April 2, 2020 02:26

Refactor the selectIndexByProximity function to use fold instead of a…

7d894bd

… search

pomadchin force-pushed the fix/gdal-auto-overview-strategy branch from 4390220 to 7d894bd Compare April 2, 2020 14:29

pomadchin reviewed Apr 2, 2020

View reviewed changes

echeipesh self-requested a review April 2, 2020 19:20

CHANGELOG [skip-ci]

5098585

echeipesh approved these changes Apr 6, 2020

View reviewed changes

echeipesh merged commit 7c245f3 into locationtech:master Apr 6, 2020


		import org.scalatest._

		class OverviewStrategySpec extends FunSpec with Matchers {

Conversation

moradology commented Mar 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Checklist

Notes

Uh oh!

pomadchin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pomadchin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pomadchin Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin Apr 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pomadchin Apr 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

moradology commented Mar 10, 2020 •

edited

Loading

pomadchin left a comment •

edited

Loading

pomadchin left a comment •

edited

Loading

pomadchin left a comment •

edited

Loading

pomadchin Mar 31, 2020 •

edited

Loading

pomadchin Mar 31, 2020 •

edited

Loading

pomadchin Apr 1, 2020 •

edited

Loading

pomadchin Apr 2, 2020 •

edited

Loading