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

Add discrete frechet distance #120

Closed
wants to merge 7 commits into
base: svn-trunk
from

Conversation

Projects
None yet
3 participants
@ssugiyama

ssugiyama commented Dec 7, 2016

I made a patch. Please review it.

See https://trac.osgeo.org/postgis/ticket/3677

<refname>ST_FrechetDistance</refname>
<refpurpose>Returns the Fréchet distance between two geometries. This is a measure of similarity between curves that takes into account the location
and ordering of the points along the curves. Units are in the units of the spatial reference system of the geometries.</refpurpose>

This comment has been minimized.

@strk

strk Dec 7, 2016

Member

Can you add a reference of the algorithm specification, and an explanation of how this is different from Hausdorff ?

@strk

strk Dec 7, 2016

Member

Can you add a reference of the algorithm specification, and an explanation of how this is different from Hausdorff ?

This comment has been minimized.

@strk

strk Dec 7, 2016

Member

Sorry, found the reference below, that's ok.

@strk

strk Dec 7, 2016

Member

Sorry, found the reference below, that's ok.

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Dec 7, 2016

Member

How about a single SQL definition, with a default 3rd argument ?

Member

strk commented Dec 7, 2016

How about a single SQL definition, with a default 3rd argument ?

@strk strk closed this Dec 7, 2016

@ssugiyama

This comment has been minimized.

Show comment
Hide comment
@ssugiyama

ssugiyama Dec 7, 2016

Well, that seems to be practical. I did so.

What should I do next?

ssugiyama commented Dec 7, 2016

Well, that seems to be practical. I did so.

What should I do next?

@strk strk reopened this Dec 7, 2016

@strk

This comment has been minimized.

Show comment
Hide comment
@strk

strk Dec 7, 2016

Member

Push your change (current "Changed Files" tab only shows a single signature but with no "densify" optional argument). Then make sure Travis build succeeds.

Member

strk commented Dec 7, 2016

Push your change (current "Changed Files" tab only shows a single signature but with no "densify" optional argument). Then make sure Travis build succeeds.

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 May 19, 2017

Member

This does not appear to be well guarded for lower versions.

If I compiled against GEOS 3.6.1 I get a compile error:

``

lwgeom_geos.o:lwgeom_geos.c:(.text+0x6a3): undefined reference to `GEOSFrechetDistance'

``

Also not clear why you removed the function with the 3rd proto (densfrac), What @strk meant is you keep that one but mark the last argument as optional.

Take a look at ST_Subdivide as an example that does this:

http://postgis.net/docs/manual-dev/ST_Subdivide.html

Looking at our docu it seems people are not correctly flagging optional args in documentation so took me sometime to find a good example you can follow.

Member

robe2 commented May 19, 2017

This does not appear to be well guarded for lower versions.

If I compiled against GEOS 3.6.1 I get a compile error:

``

lwgeom_geos.o:lwgeom_geos.c:(.text+0x6a3): undefined reference to `GEOSFrechetDistance'

``

Also not clear why you removed the function with the 3rd proto (densfrac), What @strk meant is you keep that one but mark the last argument as optional.

Take a look at ST_Subdivide as an example that does this:

http://postgis.net/docs/manual-dev/ST_Subdivide.html

Looking at our docu it seems people are not correctly flagging optional args in documentation so took me sometime to find a good example you can follow.

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 May 19, 2017

Member

More suggestions

  1. rename PG_FUNCTION from frechetdistance to ST_FrechetDistance. This is our new convention. You were probably copying something like HausdorffDistance. which follows our old convention.

  2. As noted earlier, your function is not guarded, you need to wrap your datum func into something like

#if POSTGIS_GEOS_VERSION < 37
lwpgerror("The GEOS version this PostGIS binary "
"was compiled against (%d) doesn't support "
"'ST_FrechetDistance' function (3.7.0+ required)",
POSTGIS_GEOS_VERSION);
PG_RETURN_NULL();
#else /* POSTGIS_GEOS_VERSION >= 37 */

#if

  1. Bring back your densifrac version, but have them handled by a single ST_FrechetDistance datum and SQL signature. I see they are handled by different GEOS functions, so I guess you could maybe set optional arg to -1 and have the code check if -1 is passed in, go thru the regular GEOSFrechetDistance and if some other value is passed in, use the GEOSFrechetDistanceDensify.

I was thinking of NULL as a value, but I think it would be cleaner to use a non NULL value for 3rd arg, so you can keep the function marked as STRICT to save cycles.

Member

robe2 commented May 19, 2017

More suggestions

  1. rename PG_FUNCTION from frechetdistance to ST_FrechetDistance. This is our new convention. You were probably copying something like HausdorffDistance. which follows our old convention.

  2. As noted earlier, your function is not guarded, you need to wrap your datum func into something like

#if POSTGIS_GEOS_VERSION < 37
lwpgerror("The GEOS version this PostGIS binary "
"was compiled against (%d) doesn't support "
"'ST_FrechetDistance' function (3.7.0+ required)",
POSTGIS_GEOS_VERSION);
PG_RETURN_NULL();
#else /* POSTGIS_GEOS_VERSION >= 37 */

#if

  1. Bring back your densifrac version, but have them handled by a single ST_FrechetDistance datum and SQL signature. I see they are handled by different GEOS functions, so I guess you could maybe set optional arg to -1 and have the code check if -1 is passed in, go thru the regular GEOSFrechetDistance and if some other value is passed in, use the GEOSFrechetDistanceDensify.

I was thinking of NULL as a value, but I think it would be cleaner to use a non NULL value for 3rd arg, so you can keep the function marked as STRICT to save cycles.

@ssugiyama

This comment has been minimized.

Show comment
Hide comment
@ssugiyama

ssugiyama May 23, 2017

Thank you for your suggestions. They help me a lot. I think I followed all of them. Please check them.

ssugiyama commented May 23, 2017

Thank you for your suggestions. They help me a lot. I think I followed all of them. Please check them.

@robe2

This comment has been minimized.

Show comment
Hide comment
@robe2

robe2 May 23, 2017

Member

@ssugiyama looks good. I'm making a minor adjustment to the regress test, cause you double-included regress_buffer_params test when it doesn't need to be in that loop. Aside from that looks good. I'll commit shortly.

Member

robe2 commented May 23, 2017

@ssugiyama looks good. I'm making a minor adjustment to the regress test, cause you double-included regress_buffer_params test when it doesn't need to be in that loop. Aside from that looks good. I'll commit shortly.

@ssugiyama

This comment has been minimized.

Show comment
Hide comment
@ssugiyama

ssugiyama May 23, 2017

@robe2 Oh, thank you so much for your kindness!

ssugiyama commented May 23, 2017

@robe2 Oh, thank you so much for your kindness!

@robe2 robe2 closed this in cae22d3 May 23, 2017

@pramsey pramsey referenced this pull request Oct 16, 2017

Closed

Add ST_FrechetDistance #3873

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