MaximumInscribedCircle regression check

[jodygarnett]

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]

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]

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]

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]

perfect, that is what I wished to confirm.

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

[dr-jts]

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

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