search

locationtech / jts

The JTS Topology Suite is a Java library for creating and manipulating vector geometry.
Other
1.99k stars 443 forks source link

MaximumInscribedCircle regression check #1072

Closed jodygarnett closed 3 months ago

jodygarnett commented 3 months ago

I noticed a small regression when updating to 1.20.0 and created this PR to capture it for discussion.

@dr-jts as shown in this test case MaximumInscribedCircle changed from getCentroid to getInternalPoint. I assume this was expected and produces a better result.

My question is if this change should consider the tolerance provided? The resulting value not longer "snaps" to the expected precision as was previously done...

dr-jts commented 3 months ago

The suggested tests reveal two changes in output. Both of the changes still match the contract of MaximumInscribedCircle, which is an approximation. It's better to condition the unit tests to provide more stable results. The tests use a very coarse tolerance for MaximumInscribedCircle. This produces results which is overly dependent on the algorithm internals (e.g. the switch from centroid to interior point).

The test can use a finer tolerance to get more stable results. It's also an option (as done here) to use a tolerance when checking the expected output - but this doesn't work for cases like testDoubleDiamond where the output is dramatically different.

jodygarnett commented 3 months ago

And about the "snapping" to provided tolerance? I was more meaning if the value previously snapped to POINT(15 5) reflecting the provided tolerance.

Did you intend that the new implementation snap to POINT(14 5)? Presently it has far greater precision than anticipated.

dr-jts commented 3 months ago

And about the "snapping" to provided tolerance?

The algorithm never provided rounding to the tolerance size. The appearance of this in the old result is just coincidence.

Rounding the result value to reflect the tolerance is a good idea, though.

jodygarnett commented 3 months ago

perfect, that is what I wished to confirm.

So the only question is if you want this test case?

dr-jts commented 3 months ago

So the only question is if you want this test case?

I don't think so, they are similar to other existing tests.