locationtech / locationtech/geotrellis

GridExtent.withResolution should guide users into constructing well formed instances

Open
#3,261 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement
Dominant language
Scala
Stars
1.4k
Forks
360
Avg merge
6h 57m
Merged PRs (30d)
12

Description

GridExtent.withResolution is unclear and could use reworking because it amplifies a core issue with GridExtent. The issue is that extent and cell sizes are represented as Double while cols and rows are represented as Integer:

  1. A GridExtent can be specified as having an extent and a specific number of cols and rows. This is fine, because we divide a double by an int and get a double cell size in each dimension
  2. A GridExtent can be specified as an extent and a CellSize. This is less fine, because we take a double and divide by a double to get a...double for cols and rows. But cols and rows are ints! So how to we choose what the right answer is for cols and rows in the type conversion?

A bit of a contrived example based on our current implementation:

val gridExtent = GridExtent[Long](Extent(0, 0, 10, 10), CellSize(5,5))
gridExtent.dimensions
res0: geotrellis.raster.Dimensions[Long] = 2x2
gridExtent.withResolution(CellSize(7,7)).dimensions
res0: geotrellis.raster.Dimensions[Long] = 1x1

In picture form, we're requesting:

IMG_0252

Note: The starting cell size CellSize(5, 5) doesn't affect the withResolution operation so it's not pictured.

We've cut off something like 25% of our extent without changing our extent to match because the requested CellSize does not cleanly divide at or near an integer boundary, as the initial CellSize of (5,5) does. If cols and rows could be Double, we'd get ~1.4286. Before the require() in @echeipesch 's #2886, the constructors of GridExtent allowed this as well.

The goal of this additional require check was to see where this issue cropped up. So far, its been thrown at the boundaries of floating point math, but in the case of withResolution, it exposed a mismatch in the rounding function used -- a bug.

Even with the fix applied where withResolution doesn't throw due to the bug, users are still capable of calling withResolution with a CellSize that constructs a GridExtent that doesn't align well with integer boundaries. As future work, we propose refactoring the current withResolution method into two methods:

  1. withResolution(cellSize: CellSize, threshold: Double) throws IllegalArgumentException: This method would keep the existing behavior by constructing a GridExtent as long as the requested CellSize is within threshold of a valid integer boundary, otherwise throw.
  2. withApproxResolution(cellSize: CellSize): This method would return a new GridExtent with the CellSize closest to the requested CellSize that gives an integer boundary.

This issue is to replace the existing withResolution with the two methods described in the prev paragraph. It remains unclear what the best threshold value would be.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reviewing GridExtent.withResolution, the GridExtent constructors, and the validation added in #2886. Resolve the threshold semantics and rounding behavior, then define and validate separate exact and approximate resolution APIs that preserve well-formed integer dimensions.

Written by the indexing model from the issue text.

Assessment

Tech stack
scala
Domain
data
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.