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

Use GEOSNode instead of UnaryUnion for ST_Node; fixes #3647 #136

Closed
wants to merge 5 commits into
base: svn-2.3
from

Conversation

Projects
None yet
5 participants
@Wassasin

Wassasin commented Aug 14, 2017

As per https://trac.osgeo.org/postgis/ticket/3647.
Probably the same solution as tested by @strk. Changes the results of the regression tests for ST_Node, but seems not to conflict with the documentation.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 14, 2017

Member
Member

strk commented Aug 14, 2017

@dbaston

This comment has been minimized.

Show comment
Hide comment
@dbaston

dbaston Aug 18, 2017

Member

GEOSNode is available in GEOS 3.4+, which has been available for over 4 years. Is GEOS 3.4 not a requirement for current versions of PostGIS?

Member

dbaston commented Aug 18, 2017

GEOSNode is available in GEOS 3.4+, which has been available for over 4 years. Is GEOS 3.4 not a requirement for current versions of PostGIS?

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 20, 2017

Member

Right now 3.3.0 or higher is required for PostGIS 2.4 and PostGIS 2.3.

I'd like to bump the requirement for PostGIS 2.4 to at least GEOS 3.4. It's too late to do that for 2.3.

Member

robe2 commented Aug 20, 2017

Right now 3.3.0 or higher is required for PostGIS 2.4 and PostGIS 2.3.

I'd like to bump the requirement for PostGIS 2.4 to at least GEOS 3.4. It's too late to do that for 2.3.

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 20, 2017

Member

oh I forgot we also agreed on sprint to bump to 3.5

https://trac.osgeo.org/postgis/ticket/3810

But I asked on mailing list just in case anyone has a serious objection:

https://lists.osgeo.org/pipermail/postgis-devel/2017-August/026302.html

Member

robe2 commented Aug 20, 2017

oh I forgot we also agreed on sprint to bump to 3.5

https://trac.osgeo.org/postgis/ticket/3810

But I asked on mailing list just in case anyone has a serious objection:

https://lists.osgeo.org/pipermail/postgis-devel/2017-August/026302.html

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 22, 2017

Member

@pramsey are you around? I wanted to accept this pull request, but I'd have to change the configure.ac to do so to make 3.5 the minimum and I think you are still working on it. If you haven't started yet, I can do that, but didn't want to disrupt your surgery.

Member

robe2 commented Aug 22, 2017

@pramsey are you around? I wanted to accept this pull request, but I'd have to change the configure.ac to do so to make 3.5 the minimum and I think you are still working on it. If you haven't started yet, I can do that, but didn't want to disrupt your surgery.

@pramsey

This comment has been minimized.

Show comment
Hide comment
@pramsey

pramsey Aug 22, 2017

Member
Member

pramsey commented Aug 22, 2017

@strk

please be careful about semantics, this is a fundamental function used by topology and I believe it is expected to retain the nodes originally in the input. Noding should only add nodes, not remove them. You can see the code keeps track of unique nodes in input before running LineMerge for exactly that purpose.

Show outdated Hide outdated liblwgeom/lwgeom_geos_node.c
Show outdated Hide outdated regress/node_expected
@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 22, 2017

Member

@Wassasin can you make the changes requested by @strk and then I think we are good to committ.

If you can do so before Monday that would be great.

Member

robe2 commented Aug 22, 2017

@Wassasin can you make the changes requested by @strk and then I think we are good to committ.

If you can do so before Monday that would be great.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 23, 2017

Member

Had you tried just dropping the LineMerge step ? (I did not)

Member

strk commented Aug 23, 2017

Had you tried just dropping the LineMerge step ? (I did not)

@Wassasin

This comment has been minimized.

Show comment
Hide comment
@Wassasin

Wassasin Aug 23, 2017

It seems that GEOSNode does not perform LineMerge:

< MULTILINESTRING((0 0,2.5 2.5),(0 5,2.5 2.5),(2.5 2.5,5 5,10 0),(10 0,11 0,12 0,20 0),(20 0,22 0),(2.5 2.5,5 0))
---
> MULTILINESTRING((0 0,2.5 2.5),(2.5 2.5,5 5,10 0),(10 0,11 0),(11 0,12 0),(12 0,20 0),(22 0,20 0),(0 5,2.5 2.5),(2.5 2.5,5 0))

My changes have been reverted, and now the only change is that GEOSNode is applied in stead of GEOSUnaryUnion. The preservation of nodes has been restored. In this case the tests need not be changed, except for the one cu_node.c case where only the lines change order and direction.

I was thinking of adding a note to the docs mentioning the changed behavior in the next major Postgis version. The docs indeed already mention that all input nodes are preserved.

Wassasin commented Aug 23, 2017

It seems that GEOSNode does not perform LineMerge:

< MULTILINESTRING((0 0,2.5 2.5),(0 5,2.5 2.5),(2.5 2.5,5 5,10 0),(10 0,11 0,12 0,20 0),(20 0,22 0),(2.5 2.5,5 0))
---
> MULTILINESTRING((0 0,2.5 2.5),(2.5 2.5,5 5,10 0),(10 0,11 0),(11 0,12 0),(12 0,20 0),(22 0,20 0),(0 5,2.5 2.5),(2.5 2.5,5 0))

My changes have been reverted, and now the only change is that GEOSNode is applied in stead of GEOSUnaryUnion. The preservation of nodes has been restored. In this case the tests need not be changed, except for the one cu_node.c case where only the lines change order and direction.

I was thinking of adding a note to the docs mentioning the changed behavior in the next major Postgis version. The docs indeed already mention that all input nodes are preserved.

@strk

strk approved these changes Aug 23, 2017

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 23, 2017

Member

thanks for the care

Member

strk commented Aug 23, 2017

thanks for the care

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 23, 2017

Member

So I'll commit today. Probably in a couple of hours. I assume that is okay.

Member

robe2 commented Aug 23, 2017

So I'll commit today. Probably in a couple of hours. I assume that is okay.

strk pushed a commit that referenced this pull request Aug 23, 2017

Reduce likeliness of non-noded intersections when using ST_Node
Patch contributed by Wouter Geraedts (with minor adjustments to the doc)
Closes #3647
Closes ​#136

git-svn-id: http://svn.osgeo.org/postgis/trunk@15571 b70326c6-7e19-0410-871a-916f4a2858ee
@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 23, 2017

Member

hmm this did not automatically close like it was supposed to. Oh well

Member

robe2 commented Aug 23, 2017

hmm this did not automatically close like it was supposed to. Oh well

@robe2 robe2 closed this Aug 23, 2017

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 23, 2017

Member

Please don't mix github and trac ticket references. Github ones need always be full URLs

Member

strk commented Aug 23, 2017

Please don't mix github and trac ticket references. Github ones need always be full URLs

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 23, 2017

Member

I did put in a full url for github. That's why the commit reference shows on this ticket. But seems I think it expects the first close line to be for it otherwise it doesn't automatically close.

Member

robe2 commented Aug 23, 2017

I did put in a full url for github. That's why the commit reference shows on this ticket. But seems I think it expects the first close line to be for it otherwise it doesn't automatically close.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 23, 2017

Member

I see a vare reference in b0ecd88 - is github omitting the full URL from rendering ?

Member

strk commented Aug 23, 2017

I see a vare reference in b0ecd88 - is github omitting the full URL from rendering ?

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 Aug 23, 2017

Member

Yes it is and converting it to a hyperlink -- here is my commit on svn - https://trac.osgeo.org/postgis/changeset/15571

Member

robe2 commented Aug 23, 2017

Yes it is and converting it to a hyperlink -- here is my commit on svn - https://trac.osgeo.org/postgis/changeset/15571

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Aug 23, 2017

Member
Member

strk commented Aug 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment