Skip to content

Fix numerical errors - #3520

Closed
jdries wants to merge 5 commits into
locationtech:masterfrom
VitoTAP:grid_flooring
Closed

Fix numerical errors#3520
jdries wants to merge 5 commits into
locationtech:masterfrom
VitoTAP:grid_flooring

Conversation

@jdries

@jdries jdries commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

Overview

When working in epsg:4326 coordinates, I recently observed separate bugs caused by the same problem. Basically transformations between geographical coordinates and grid coordinates where wrong due to floating point imprecision.
In both cases, grid coordinates were calculated as doubles and resulted in e.g. 4.9999999 but were done floored (toLong) resulting in 4.

This resulted in an actual pixel shift in one case, and in another case a missing row of pixels at the bottom of a saved geotiff.

Checklist

  • ./CHANGELOG.md updated, if necessary. Link to the issue if closed, otherwise the PR.
  • Unit tests added for bug-fix or new feature

Notes

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

Closes #XXX

@jdries
apply rounding/flooring consistently
bb49512
@jdries
fix wrong offset due to limited numerical precision
8f24742 
values like 4.9999999999 were actually floored to 4, whereas it should be 5, caused pixel shifts in output
@jdries

jdries commented Sep 8, 2023

Copy link
Copy Markdown
Contributor Author

Found a simple test that exposes the issue. It's all about gridextents that are in fact aligned, but due to very small numerical differences, things go wrong. Still have to figure out where and how to integrate this as part of geotrellis unit tests.

  val targetBounds = GridBounds(55L,103,55+73,103+111)
   val ge = GridExtent[Long](Extent(1.8973214288275528, 49.352678585684544, 6.299107143119465, 51.7276785856839), CellSize(0.008928571428584,0.008928571428568996))
   val targetExtent = ge.extentFor(targetBounds)

   //second, aligned extent based on a LayoutDefinition
   val ge2 = GridExtent[Long](Extent(2.3883928573996727, 49.66517858568254, 3.5312500002584244, 50.80803572854129), CellSize(0.008928571428583998,0.008928571428583998))

   //gridextents have to be aligned for test to be valid
   //basically copy of geotrellis.layer.LayoutTileSource#requireGridAligned
   requireGridAligned(ge,ge2)
   val resultingBounds = ge2.gridBoundsFor(targetExtent)


   assertEquals(targetBounds.height,resultingBounds.height)
   assertEquals(targetBounds.width,resultingBounds.width)

@pomadchin

pomadchin commented Sep 8, 2023

Copy link
Copy Markdown
Member

@jdries a nice find! Could you add this unit test?

@jdries
jdries marked this pull request as ready for review September 10, 2023 15:19
@jdries jdries mentioned this pull request Sep 11, 2023
Closed
pomadchin
pomadchin requested changes Sep 13, 2023
View reviewed changes

@pomadchin pomadchin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔥 Thx for a nice PR, but tests don't compile! Once the CI is green, I'll merge it.

Comment thread raster/src/test/scala/geotrellis/raster/GridExtentSpec.scala
Comment on lines +194 to +195
targetBounds.height should be resultingBounds.height
targetBounds.width should be resultingBounds.width

@pomadchin pomadchin Sep 13, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tests don't compile!

Suggested change
targetBounds.height should be resultingBounds.height
targetBounds.width should be resultingBounds.width
targetBounds.height shouldBe resultingBounds.height
targetBounds.width shouldBe resultingBounds.width

@pomadchin pomadchin mentioned this pull request Sep 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pomadchin pomadchin pomadchin requested changes

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jdries @pomadchin