Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix numerical errors #3520

Closed
wants to merge 5 commits into from
Closed

Conversation

jdries
Copy link
Contributor

@jdries jdries commented Sep 8, 2023

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

values like 4.9999999999 were actually floored to 4, whereas it should be 5, caused pixel shifts in output
@jdries
Copy link
Contributor Author

jdries commented Sep 8, 2023

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
Copy link
Member

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

Copy link
Member

@pomadchin pomadchin left a comment

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 on lines +194 to +195
targetBounds.height should be resultingBounds.height
targetBounds.width should be resultingBounds.width
Copy link
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants