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

Adding Frechet distance And Hausdorff distance algorithms to calculate Simmilarity Between Geometries #490

Merged
merged 40 commits into from Sep 14, 2018

Conversation

Projects
None yet
6 participants
@yaghya
Copy link
Contributor

yaghya commented Jun 18, 2018

Implemented Frechet distance and Hausdorff distance algorithms for Linestrings. Checked the algorithms for different Coordinate System like Cartesian , Geographic and Spherical_Equatorial.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Jun 18, 2018

Thanks!

Please change the file names to reflect the ones we already have in Boost.Geometry (lower-case with underscores).

In the future commits also add tags at the beginning telling which part of the library you're modifying. In this case it could be [algorithms] Adding Frechet Distance And Haudorff Distance algorithms

boost::geometry::detail::throw_on_empty_input(ls2);

size_type const a = boost::size(ls1);
size_type const b = boost::size(ls2);

This comment has been minimized.

@awulkiew

awulkiew Jun 18, 2018

Member

size_type of Linestring1 could be different than size_type of Linestring2.

This comment has been minimized.

@awulkiew

awulkiew Jun 18, 2018

Member

Right but you have to use something in coup_map. You could use the greater type or leave it as it is for now. In practice this should be ok.

This comment has been minimized.

@yaghya

yaghya Jun 20, 2018

Contributor

I have updated the code and defined size_type1 for Linestring1 and size_type2 for Linestring2. And also used both these size_types in creating coup_mat.

typedef typename boost::range_size<Linestring1>::type size_type;

result_type dis_max=0,dis_min;
result_type const infinite = std::numeric_limits<double>::max();

This comment has been minimized.

@awulkiew

awulkiew Jun 18, 2018

Member

infinite is not needed right? Also using std::numeric_limits<double>::max() is wrong since user-define coordinate type can support values greater than that.

This comment has been minimized.

@yaghya

yaghya Jun 20, 2018

Contributor

I forgot to remove it. I have removed it in the updated code.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Jun 18, 2018

The code looks good. Keep it up!

@yaghya yaghya force-pushed the yaghya:feature/similarity branch from 566d49e to 2d9e66c Jun 20, 2018

//Print CoupLing Matrix
for(size_type i = 0; i <a; i++)
{
for(size_type j = 0; j <b; j++)

This comment has been minimized.

@vissarion

vissarion Jun 21, 2018

Contributor

"{" missing in for loop

This comment has been minimized.

@yaghya

yaghya Jun 25, 2018

Contributor

Updated.

(std::max)(coup_matrix(i-1,j),geometry::distance(range::at(ls1,i),range::at(ls2,j),strategy));
else if(i>0 && j>0)
coup_matrix(i,j)=
(std::max)((std::min)((std::min)(coup_matrix(i,j-1),coup_matrix(i-1,j)),coup_matrix(i-1,j-1)),geometry::distance(range::at(ls1,i),range::at(ls2,j),strategy));

This comment has been minimized.

@vissarion

vissarion Jun 21, 2018

Contributor

wrap

This comment has been minimized.

@yaghya

yaghya Jun 25, 2018

Contributor

Updated.

@@ -0,0 +1,177 @@
/*

This comment has been minimized.

@vissarion

This comment has been minimized.

@yaghya

yaghya Jun 21, 2018

Contributor

Ok, I will update the copyright.

@vissarion

This comment has been minimized.

Copy link
Contributor

vissarion commented Jun 21, 2018

Thanks, the code is good indeed. I added a few (stylistic) comments. You can have a look at this https://github.com/boostorg/geometry/wiki/Guidelines-for-Developers for more.

for(size_type2 j=0;j<b;j++)
{
result_type dis_min;
bool is_dis_min_set = false;

This comment has been minimized.

@awulkiew

awulkiew Jun 21, 2018

Member

This part of the code is very similar to the algorithm for Ls-Ls. To avoid duplication of code you should make a helper function from this part and call it inside the algorithm for Ls-Ls and Ls-MLs. So a function taking two ranges of points, PtPtDistanceStrategy and the current max distance (which will be updated by the function, so both input and output variable).

And later do the same with MLs-MLs because a part of algorithm there will be the same as this algorithm for Ls-MLs.

You could name these helper functions range_range(), and range_multirange() because they could also be used for MultiPoint-MultiPoint so you should use some generic (not geometry specific) name.

You could also have point_range() function (this would be the most inner loop you have in algorithms). And you could call it inside a loop in range_range() and directly in Point-MultiPoint algorithm.

This comment has been minimized.

@awulkiew

awulkiew Jun 21, 2018

Member

Or you could simply call linestring_linestring() here. Maybe it's even better.

Note that you could use the same function for MultiPoint-MultiPoint, so still renaming it to range_range() makes sense.

This comment has been minimized.

@yaghya

yaghya Jun 22, 2018

Contributor

I have Updated hausdorff_distance, now I am calling linestring_linestring().

@yaghya

This comment has been minimized.

Copy link
Contributor

yaghya commented Jun 23, 2018

I have Update haudorff_distance for MultiLinestring_MultiLinestring and made the code generic by using range_range() and range_multi_range(). I will extend the code for point-multi_point and multi_point, soon.

@mkaravel

This comment has been minimized.

Copy link
Member

mkaravel commented Jun 27, 2018

I am a bit confused here about what the goal is. I have not looked at the Frechet distance, but looking at the Hausdorff distance it seems that what is computed is the Hausdorff distance between the vertices of the two geometries. Is that the goal?

I am asking this since the Hausdorff distance between two linestrings is not the same as the Hausdorff distance of their vertices. In fact, if one wants to compute it correctly, the Voronoi diagram of at least one of the linestrings needs to to involved/computed. There are of course approximate iterative ways for computing an approximation.

Here is an example pair of linestrings that can be used as a test case: LINESTRING(1 0,0 0,0 100,1 100) and LINESTRING(2 0,3 0,3 100,2 100). For these two linestrings the actual Hausdorff distance is 3. Looking at the vertices only we get a distance of 2. There are other examples/configurations where the "discrete" Hausdorff distance computed via the vertices is arbitrarily larger than the actual Hausdorff distance.

So the real question here is what does this PR intend to compute?

@vissarion

This comment has been minimized.

Copy link
Contributor

vissarion commented Jun 27, 2018

@mkaravel indeed, this PR implements discrete versions of both Hausdorff and Frechet distances (i.e. considering only vertices of geometries, in Frechet case their sequence too). I think it would be clearer to add the word discrete in the title of the PR as well as in the names for the algorithms in the code. This will avoid future confusions.

On the other hand, implementing (a both efficient and more accurate approximation of) the continuous versions sounds interesting as a future work but also much more involved.

If anyone is aware of any well known open-source code for the continuous versions of those algorithms it would be useful to post it here. It seems that PostGIS (that uses GEOS) also implements the discrete versions. See: 1 2

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Jun 27, 2018

Most importantly it's a GSOC project so should be considered as work in progress. PR's title is derived from the GSOC's project title.

One of the things we should figure out is how the interface should look like, how these algorithms should be semantically represented, at which level of abstraction. So function names are also temporary I think.

#include <boost/geometry/geometries/point_xy.hpp>
#include <boost/geometry/geometries/polygon.hpp>

using namespace boost::geometry;

This comment has been minimized.

@awulkiew

awulkiew Aug 14, 2018

Member

Remove, use bg:: instead.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Aug 14, 2018

In addition to the comments above the frechet distance cases are failing with gcc-5.5 and clang-3.8. For coordinate types float and int the result is different than expected probably due to the fact that you computed it for double, e.g.:

difference{1.02932e-05} between h_distance{333961.438} and expected_frechet_distance{333958} exceeds 0.001%
difference{0.00114489} between h_distance{0.05235987755982989} and expected_frechet_distance{0.052299999999999999} exceeds 0.001%

So either test coordinate types separately (with different expected results) or only test double (remove int and float) or hide them behind #ifdef. I'm ok with testing only double. Note also that testing int in geographic and spherical doesn't have sense when degrees or radians are represented directly.

Unless there is something wrong with the algorithm. And in this case the algorithm must be fixed.

In the future make sure that the code works before pushing it.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Aug 15, 2018

Great, thanks!

Many wierd indentations appeared in the test files. It looks like spaces were automatically changed to tabs or something. Is it on purpose? You should use 4-space indentations for Boost.Geometry.

There is some conflict with develop probably after my change in tests. Do you know how to resolve it?

git checkout develop
git pull
git checkout feature/similarity
git merge develop
git push

should be enough.

Yaghyavardhan singh khangarot and others added some commits Aug 15, 2018

@@ -79,5 +79,7 @@ build-project overlay ;
build-project perimeter ;
build-project relate ;
build-project set_operations ;
build-project similarity;

This comment has been minimized.

@awulkiew

awulkiew Aug 15, 2018

Member

One more thing. A space between word similarity and ; is needed.

boost::geometry::read_wkt("LINESTRING(0 0,1 1,1 2,2 1,2 2)", linestring1);
boost::geometry::read_wkt("LINESTRING(1 0,0 1,1 1,2 1,3 1)", linestring2);
double res;
res = boost::geometry::frechet_distance(linestring1,linestrign2);

This comment has been minimized.

@awulkiew

awulkiew Aug 15, 2018

Member

Wrong function name.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Aug 15, 2018

Ok, please let me know when you make sure that everything compiles and runs properly with b2, so when run in BOOST_ROOT\libs\geometry:

../../b2 test/algorithms
../../b2 doc/src/examples/algorithms
@yaghya

This comment has been minimized.

Copy link
Contributor

yaghya commented Aug 15, 2018

I have Updated the examples and Jamfile. And now both

../../b2 test/algorithms
../../b2 doc/src/examples/algorithms

runs properly .

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Aug 16, 2018

It works for me as well. Good job.

I think this PR is good enough to be merged. What do you think @barendgehrels @mloskot @vissarion ?

@vissarion
Copy link
Contributor

vissarion left a comment

Thanks, great job! I have few minor comments. I am ok with merging.

[heading Available Strategies]
\* [link geometry.reference.strategies.strategy_distance_pythagoras Pythagoras (cartesian)]
\* [link geometry.reference.strategies.strategy_distance_haversine Haversine (spherical)]

This comment has been minimized.

@vissarion

vissarion Aug 21, 2018

Contributor

There are also geographic strategies available right?

This comment has been minimized.

@yaghya

yaghya Aug 26, 2018

Contributor

For the Geographic Coordinate system, only the default strategy works. I have not tested the code with vincenty and andoyer strategy yet.

This comment has been minimized.

@awulkiew

awulkiew Aug 26, 2018

Member

What do you mean by that? The algorithm doesn't compile? The result is incorrect? Or documentation cannot be built?

Right now geographic strategies are not mentioned at all in the documentation I think. So if we wanted to add link here we should first document geographic Pt-Pt distance strategy.

This comment has been minimized.

@vissarion

vissarion Aug 27, 2018

Contributor

@yaghya It would be good to add some tests using the other strategies too. andoyer is the default so I guess you have to test vincenty and thomas.

Regarding documentation what about adding text (without link) to notify the user that geographic strategies can be used even if they are currently undocumented.

This comment has been minimized.

@awulkiew

awulkiew Aug 27, 2018

Member

If you decide to add more tests use:

strategy::distance::geographic<strategy::andoyer>()
strategy::distance::geographic<strategy::thomas>()
strategy::distance::geographic<strategy::vincenty>()

Regarding documentation I think it's a bad idea. If we agree that the current interface for geographic CS will not change the documentation has to be updated consistently. In that case all geographic strategies, srs spheroid and geographic policies (andoyer, thomas, vincenty) has to be documented and links to strategies added in every algorithm, not only here.

This comment has been minimized.

@yaghya

yaghya Aug 30, 2018

Contributor

Added test cases Geographic Strategies and Updated the documentation.

This comment has been minimized.

@awulkiew

awulkiew Aug 30, 2018

Member

This change only makes the docs more confusing, not less.



/*!
\brief calculate discrete frechet distance between two geometries using specified strategy

This comment has been minimized.

@vissarion

vissarion Aug 21, 2018

Contributor

It should be useful to state that current implementation only support certain types of geometries, i.e. linestrings.

// Algorithm overload using explicitly passed Pt-Pt distance strategy

/*!
\brief calculate discrete hasudorff distance between two geometries using specified strategy

This comment has been minimized.

@vissarion

vissarion Aug 21, 2018

Contributor

Similar to Frechet distance useful to state that current implementation only support certain types of geometries and geographic strategies are available.

@awulkiew awulkiew requested a review from barendgehrels Aug 26, 2018

\ingroup discrete_frechet_distance
\tparam Geometry1 \tparam_geometry
\tparam Geometry2 \tparam_geometry
\param geometry1 Input geometry
\param geometry2 Input geometry
\qbk{[include reference/algorithms/discrete_frechet_distance.qbk]}
\qbk{[inc\* more (currently extensions): Vincenty\, Andoyer (geographic)
lude reference/algorithms/discrete_frechet_distance.qbk]}

This comment has been minimized.

@awulkiew

awulkiew Aug 30, 2018

Member

I doubt the docs can be built now.

@@ -257,7 +257,7 @@ struct discrete_hausdorff_distance<multi_linestring1,multi_linestring2,multi_lin
// Algorithm overload using explicitly passed Pt-Pt distance strategy

/*!
\brief calculate discrete hausdorff distance between two geometries ( currently works for LineString-LineString, MultiPoint-MultiPoint, Point-MultiPoint, MultiLineString-MultiLineString ) using specified strategy
\brief calculate discrete hasudorff distance between two geometries ( currently works for LineString-LineString, MultiPoint-MultiPoint, Point-MultiPoint, MultiLineString-MultiLineString ) using specified strategy

This comment has been minimized.

@awulkiew
@@ -269,10 +269,11 @@ struct discrete_hausdorff_distance<multi_linestring1,multi_linestring2,multi_lin
\qbk{distinguish,with strategy}
\qbk{[include reference/algorithms/discrete_hausdorff_distance.qbk]}
\qbk{
\qbk{

This comment has been minimized.

@awulkiew

awulkiew Aug 30, 2018

Member

Why is this space needed?

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Aug 30, 2018

FYI, I'm waiting with merging this PR because I want to hear from @barendgehrels. Not because I expect you to improve it. Well, at least this was the case before the recent changes because now it's no longer ready to be merged.

@yaghya

This comment has been minimized.

Copy link
Contributor

yaghya commented Aug 30, 2018

Sorry for the silly mistakes. I have Updated the doc and now both

../../b2 test/algorithms
../../b2 doc/src/examples/algorithms

runs properly.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Sep 14, 2018

Ok, we waited long enough. I'm merging the PR. Thanks for your contribution!

@awulkiew awulkiew merged commit 37e6eea into boostorg:develop Sep 14, 2018

@awulkiew awulkiew added this to the 1.69 milestone Sep 14, 2018

@barendgehrels

This comment has been minimized.

Copy link
Collaborator

barendgehrels commented Sep 15, 2018

Sure, sorry @awulkiew , I was on holiday at the time you announced to wait for me.
@yaghya thanks a lot for the new algorithm!
And thanks for merging.
Regards, Barend

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Sep 16, 2018

Ok, thanks!

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