It looks like we're computing cols / rows via the ceil operation but validating it via the round function.
In short, yes. And GridExtent currently operates in its constructor using round
so in order to remain consistent, we should use that elsewhere when we construct new GridExtents, like in withResolution
and combine
.
The long version is that 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:
- 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
- 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:
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
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:
withResolution(cellSize: CellSize, threshold: Double) throws IllegalArgumentException
: This method would keep the existing behavior by constructing aGridExtent
as long as the requestedCellSize
is withinthreshold
of a valid integer boundary, otherwise throw.withApproxResolution(cellSize: CellSize)
: This method would return a newGridExtent
with theCellSize
closest to the requestedCellSize
that gives an integer boundary.