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

Geometry equality in Java tests #178

Closed
3 tasks
springmeyer opened this issue Jun 21, 2024 · 6 comments
Closed
3 tasks

Geometry equality in Java tests #178

springmeyer opened this issue Jun 21, 2024 · 6 comments
Labels

Comments

@springmeyer
Copy link
Collaborator

springmeyer commented Jun 21, 2024

Context

JTS geometries are used for the Java geometry representation. JTS geometries have sophisticated equality methods as is needed given the complexity of geometry storage:

See:

Currently in our tests we use Junit assertEqual.

Problem

The results of assertEqual(mltgeometry, mvtgeometry) do not match always match assertEquals(true, mltgeometry.equals(mvtgeometry))

Which raises the question of what assertEqual is doing under the hood? Is it converting to strings to compare objects? Is it using geom.equals? Something else?

Next steps

Overall goal here is to ensure that our Java tests are correctly asserting that MVT geometries can be perfectly round-tripped between MVT and MLT. In other words, that their representations are identical after both are decoded into a given in-memory representation.

I think this is already the case, but we should double-check/ensure it is by ensuring our tests are correctly asserting on equality.

So, how about?

  • Move to assertEquals(true, mltgeometry.equals(mvtgeometry)) or assertTrue( mltgeometry.equals(mvtgeometry))?
    • Fix failures that arise
  • Or consider some other approach?

/cc @mactrem @ebrelsford

@springmeyer
Copy link
Collaborator Author

Perhaps also @msbarry has some insight here? Testing issues that lead to this ticket came from #168 (comment)

@springmeyer
Copy link
Collaborator Author

Started to look into this a bit. Found that mvtGeometry.equals(mltGeometry) throws while mvtGeometry.equalsExact(mltGeometry) does not throw (and passes). The difference is that the former checks for topological equality while the latter checks for structural equality. I think what may be happening is that the geometries are equal/identical (and therefore the structural equality test passes as it should) but they are invalid in some way so that topological equality test cannot cope and fails to finish.

@springmeyer
Copy link
Collaborator Author

they are invalid in some way

Visualizing the WKT https://wktmap.com in shows that most of the failing geometries are polygons which have collapsed to zero area lines. So this problem appears to be coming from either the input geometries (likely) or the handling of them in https://github.com/ElectronicChartCentre/java-vector-tile.

So, I think the solution here is to use mvtGeometry.equalsExact(mltGeometry)

@springmeyer
Copy link
Collaborator Author

#180 implements explicit structural equality check.

@msbarry
Copy link
Collaborator

msbarry commented Jun 22, 2024

JTS geometry does something kind of weird, it has 2 methods public boolean equals(Geometry g) (alias for equalsTopo) and public boolean equals(Object o) (alias for equalsExact). Junit assertEquals should be using the latter, but if you just write geometry.equals(otherGeometry) it will resolve to the former with different behavior.

For a exact comparison I think you'd want assertEquals(geom1, geom2) but for planetiler I wanted looser comparisons in a few places so I added an assertTopologicallyEquivalentFeature method that compared using equalsTopo

@springmeyer
Copy link
Collaborator Author

@msbarry great, thanks for the followup info. Above you'll see I implemented an explicit structural equality test (aka Geometry.equalsExact( Geometry g )) in #180. I'm happy with doing this and not depending on what assertEquals matches to, but it is great to know you think it matches to Geometry.equals( Object o ).

springmeyer added a commit that referenced this issue Jun 25, 2024
Currently we do not fully test all these combinations:

 - Vectorized path + advanced encodings
 - Vectorized path + without advanced encodings
 - Non-Vectorized path + advanced encodings
 - Non-Vectorized path + without advanced encodings
 
This PR starts testing all those paths.

The code coverage changes are:

 - **overall:** `36%`to `45%`
 - **mlt.converter:** `64%` to `85%`
 - **mlt.decoder:** `55%` to `71%`

In addition this PR includes:

- Some refactoring in TestUtils.java to share more code between
vectorized and non-vectorized
- Improvements to TestUtils.java around geometry comparisons (refs #178)
 - Improvements to TestUtils.java around property comparisons

Finally, this PR does not attempt to solve any bugs. Rather it finds,
isolates, and writes tests harnesses to capture them in code. Once bugs
are solved in the future it should be easy to update the tests
accordingly.

TODO:
- [x] ~This PR uncovered a number of bugs that do not have open issues
and did not have tests that hit them until now. So I need to create
following issues to capture fixing these.~
     - Added:
        - #181
        - #182 
        - #183 
        - #184
        - #185
        - #186
- [ ] Property nesting in MLT makes comparison to MVT difficult so I
disabled it in this test refactor because comparison to MVT is what the
decoder tests rely on. My sense is that standalone unit tests of nesting
might be more appropriate than including nesting in the decoder tests.
Do others agree?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants