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 inscribed circle initialization #1225

Merged
merged 2 commits into from
Jan 13, 2025
Merged

Fix inscribed circle initialization #1225

merged 2 commits into from
Jan 13, 2025

Conversation

unjambonakap
Copy link
Contributor

Fix a bug in MaximumInscribedCircle that gives a bad result

import shapely, shapely.constructive
import numpy as np
poly = shapely.from_geojson('{"type":"Polygon","coordinates":[[[0.0,-10.0],[-7.07107,-7.07107],[-10.0,0.0],[-7.07107,7.07107],[0.0,10.0],[7.07107,7.07107],[10.0,0.0],[7.07107,-7.07107],[0.0,-10.0]]]}')

u = shapely.constructive.maximum_inscribed_circle(poly, tolerance=0.1)
a, b = np.array(u.coords)
print(a,b,np.linalg.norm(a-b))

Actual:
[0. 3.535535] [-2.28553181 9.05330273] 5.972387826209451

Expected:
[0. 0.] [ 8.53553655 -3.53553126] 9.238796754579768

@pramsey
Copy link
Member

pramsey commented Jan 13, 2025

As you can see, this fails regression, though only regression tests on invalid inputs. Unfortunate!

  • in test 5, the result moves from the middle of the (invalid) geometry to the end, which is "less good" though again we are talking about invalid input and a zero-radius output...
  • similar story in test 7, moving from the middle to the end
---> group: geos::algorithm::construct::MaximumInscribedCircle, test: test<5>
     problem: assertion failed
     failed assertion: `x coordinate does not match: expected `1.5000000000000000e+02` actual `1.0000000000000000e+02` with precision `1.0000000000000000e-02``

---> group: geos::algorithm::construct::MaximumInscribedCircle, test: test<7>
     problem: assertion failed
     failed assertion: `x coordinate does not match: expected `2.0000000000000000e+00` actual `1.0000000000000000e+00` with precision `1.0000000000000000e-02``

@pramsey
Copy link
Member

pramsey commented Jan 13, 2025

The JTS tests seems to also expect the result to be the same as the centroid point, so I am not at all clear on why when you initialize to the centroid you then get back a non-centroid answer. That seems like something we should understand before just changing the regression answers.

@unjambonakap
Copy link
Contributor Author

The fact that it used to hit the centroid is purely by luck:
Before test5:

  • First cell distance is bad (because computed with (0,0), outside)
  • first iteration, gets improved with 0 (this happens to be the centroid)
  • no more improvements

Now:

  • first cell is the best one, attributed to the computed interior point (on the boundary, 100 100)(

pramsey added a commit that referenced this pull request Jan 13, 2025
@pramsey pramsey merged commit 63fb272 into libgeos:main Jan 13, 2025
pramsey added a commit that referenced this pull request Jan 13, 2025
pramsey added a commit that referenced this pull request Jan 13, 2025
pramsey added a commit that referenced this pull request Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants