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

Possible backport for concaveHull #1034

Closed
mwtoews opened this issue Jan 25, 2024 · 7 comments
Closed

Possible backport for concaveHull #1034

mwtoews opened this issue Jan 25, 2024 · 7 comments

Comments

@mwtoews
Copy link
Contributor

mwtoews commented Jan 25, 2024

As reported shapely/shapely#1971 there is an issue with this example:

$ ./geos-3.12.0/bin/geosop -a "MULTIPOINT ((175682.210293 505233.365178), (175682.210295 505233.365177), (175684.375108 505254.524644), (175684.375109 505254.524642), (175683.226468 505243.284373))" concaveHull 0.0
Segmentation fault (core dumped)

this was recently resolved for GEOS 3.12.1 with this expected result:

$ ./geos-3.12.1/bin/geosop -a "MULTIPOINT ((175682.210293 505233.365178), (175682.210295 505233.365177), (175684.375108 505254.524644), (175684.375109 505254.524642), (175683.226468 505243.284373))" concaveHull 0.0
POLYGON ((175682.210293 505233.365178, 175684.375108 505254.524644, 175684.375109 505254.524642, 175683.226468 505243.284373, 175682.210295 505233.365177, 175682.210293 505233.365178))

The mystery is how this was resolved, as there is nothing obvious in NEWS.md to backport this fix for older branches (e.g. a forthcoming 3.11.4). Does this look familiar to anyone?

@mwtoews
Copy link
Contributor Author

mwtoews commented Jan 25, 2024

I should also note that this is not an issue with JTS, but here is JTS TestBuilder for the viz of input points and thin polygon result:
image

@dr-jts
Copy link
Contributor

dr-jts commented Jan 25, 2024

This was fixed by #953, which was not backported to 3.11 (can't remember why now).

Aside: it would be nice if GH allowed tagging issues with code files/lines that are known to be related (e.g. the cause of the issue, or affected by it).

@mwtoews
Copy link
Contributor Author

mwtoews commented Jan 26, 2024

Testing a backport:

But is it a good idea to backport? See #1035 for candidate.

@pramsey
Copy link
Member

pramsey commented Jan 26, 2024

I think fixes are good. There will be some small behaviour changes, but the tradeoff is more working cases... seems like a win to me.

@dr-jts
Copy link
Contributor

dr-jts commented Jan 26, 2024

I'm in favour of this too. Thanks, @mwtoews .

@mwtoews
Copy link
Contributor Author

mwtoews commented Jan 26, 2024

Thanks all, I'll do a better review later today before merging (this was 5 mins to prepare). Also reevaluate if any other backports are simple and feasible.

@mwtoews
Copy link
Contributor Author

mwtoews commented Jan 27, 2024

Also xref to #946 and shapely/shapely#1873

@mwtoews mwtoews closed this as completed Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants