[experimental] update test-demo with version with lambdas #1146

barendgehrels · 2023-05-08T20:37:06Z

Hi, this is an experimental version of partition using lambdas.

For demonstration and evaluation purposes.

I'm curious to your findings.

barendgehrels · 2023-05-08T20:38:19Z

test/robustness/within/partition_within.cpp

+ return true;
+ },
+ box_visitor
+ );


This shows how the function could be used.
5 lambdas (2 expand, 2 overlaps, 1 main) plus an optional lambda (called per box),
instead of the earlier policies with apply

The program itself creates SVG's like this:

See also https://barendgehrels.blogspot.com/2023/05/partition-with-boostgeometry.html

A lot better! I prefer function objects over objects with apply() function.

One nitpick. This is not quadtree, it's kdtree, a node in quadtree is divided into 4 children, here a division is done into 2, once per dimension. Furthermore partition is k-dimensional, quadtree is 2-dimensional.

Side note, I was thinking about redesigning the API for a long time and use features from the rtree. Note e.g. that in your example you shouldn't be forced to pass function objects defining how to expand a box or perform intersection. These operations are always done the same way. We probably also never use box visitors, besides debugging or visualization so box visitor could be hidden as well. The only additional information in your example that's needed is the way of getting a box from item (taking point from point should work by default). In rtree this is done by IndexableGetter. There are also additional things like the type of operation (in this case intersects) and strategies (for defining CS) but they could be optional as well. I'm not saying to do this, only sharing what I was thinking about.

Thanks!

I think it's not exactly a kdtree either. You are right, we divide into halves and not into 4. But always at the middle, in kdtree that it is not necessarily the case. Anyway, I can rename it of course. But it cannot be called partition... (unless we replace it).
Plus we have those additional boxes for cases where geometries exceed the halves. But sure, they are not part of the quadtree either.

I see your point. Yes, taking the box from an object is usually enough. Or point. But it could be done in a different way as well. For example: a box with a margin (for (max) distance calculations). It's flexible now. But that flexibility is probably used rarely, if at all. We use partition only internally, I think. It was introduced before our index so at that moment it was not a choice. So yes, it can be a bit simpler. The box visitor is indeed only used for debugging (at least at this moment).

Anyway, additionally, if we go this way, we don't need those visitor-adaptor(s), we can deprecate the old implementation and let the new one immediately use function objects.

Splitting planes can be placed at various locations in a kd-tree. Classic approach is to place them at the median object. Another strategy is to cut empty space out. There is also version using surface area heuristic. There are probably more. It depends on a use case. Splitting in half is probably a poor choice in general good only for objects distributed uniformly but such tree is still kd-tree.

Btw, in kd-tree splitting doesn't have to be done one dimension after another. The space can be split e.g. in X axis cutting out empty space and then again in X axis at the middle object, etc. But no matter what strategy is used this is still kd-tree.

Regarding quadtrees. There are versions of quadtrees that have splitting planes at different locations than geometric middle of a node. AFAIR there are also quadtrees with child nodes partially overlapping each other which makes them similar to rtrees.

Obviously partition is neither quadtree nor kd-tree because it's not a data structure.

You're completely right, it's not a data structure. Thanks for the backgrounds. I will just keep the partition name.
As a work-title it's now temporarily lambda_partition.
I will not merge this PR. It is a discussion piece. If we agree and things are clear, I'll change the original (copy, and deprecate the old one)

It's used internally around 12 times.
Unfortunately it's not in namespace detail so officially it is part of the interface.

awulkiew · 2023-05-09T08:07:45Z

test/robustness/within/quadtree_partition.hpp

+>
+bool quadtree_partition(ForwardRange1 const& forward_range1,
+ ForwardRange2 const& forward_range2,
+ const std::function


I'd avoid using std::function because it's heavy. Why did you decide to use it over a function object?

The compiler error messages are clearer.
I was not aware that it is heavier.

awulkiew · 2023-05-09T08:13:12Z

test/robustness/within/quadtree_partition.hpp

+template <typename F>
+struct adapt_partition_visitor
+{
+ const F& m_f;
+
+ explicit adapt_partition_visitor(const F& f)
+ : m_f(f)
+ {}
+
+ template<typename T1, typename T2>
+ bool apply(T1 const& i1, T2 const& i2) const
+ {
+ return m_f(i1, i2);
+ }
+};


All of the adaptors could probably be replaced with one simply forwarding parameters to function object:

template <typename F> struct adapt_partition_visitor { F m_f; explicit adapt_partition_visitor(F&& f) : m_f(std::move(f)) {} template <typename ...Ts> decltype(auto) apply(Ts&& ...is) const { return m_f(std::forward<Ts>(is)...); } };

Indeed, works perfectly. Thanks!
✔️

vissarion

Thanks, this is cool! I am OK with merging now.
I will have a deeper look next week.
Regarding the name I also agree it is neither quad nor kd-tree (it is not a tree at all of course and it actually combines the default splitting strategy "cut in middle" of quad trees with the kd-tree per-dimension partitioning). So I think it is better to just call it "partitioning".

vissarion · 2023-05-12T12:39:06Z

test/robustness/within/Jamfile

@@ -0,0 +1,17 @@
+# Boost.Geometry (aka GGL, Generic Geometry Library)
+# Robustness Test - convex_hull


this is not convex hull, typo?

Indeed, I copied another file and forgot to adapt the title.
✔️ fixed

Thanks, this is cool! I am OK with merging now. I will have a deeper look next week. Regarding the name I also agree it is neither quad nor kd-tree (it is not a tree at all of course and it actually combines the default splitting strategy "cut in middle" of quad trees with the kd-tree per-dimension partitioning). So I think it is better to just call it "partitioning".

Renamed now to partitioning (currently in namespace experimental)

vissarion · 2023-05-12T12:43:11Z

test/robustness/within/lambda_partition.hpp

+ detail::partition::include_all_policy
+ >::apply(forward_range1, forward_range2, av,
+ aev1, aov1,
+ aev2, aov2, 16, apbv);


maybe add a comment that 16 is the end of the iteration (the max number of points in a partition box)

I changed it to an options structure, as I planned earlier.
Is something like this fine?
✔️

barendgehrels · 2023-09-29T12:51:18Z

I updated this PR:

adding a lambda version over one range as well (like the original had)
applying the lambda version in buffer, which nicely removes many helper structures.

It's still in namespace experimental (so don't merge yet), I can't put it in namespace geometry because it conflicts with the structure partition. Any suggestions?

barendgehrels · 2023-09-29T12:53:08Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

+ {
+ return turn.is_turn_traversable && ! turn.within_original;
+ }
+ };


Doing this, I noticed that we sometimes use the include_turn_policy but sometimes have it in the get_box/overlaps functors. For that reason, if this functionality is always there, we could also choose to get rid of these options.
(the one left is the default partition size but we can specify it as an optional parameter).
What do you think?

I omitted it, it was really just about expanding the bounding box.

barendgehrels · 2023-09-29T12:54:21Z

include/boost/geometry/algorithms/detail/partition_lambda.hpp

+template
+<
+ typename BoxType,
+ typename ForwardRange,


This version is added, it uses only one ForwardRange which is partitioned with itself

…bdas

barendgehrels · 2023-10-04T13:35:50Z

I updated this PR:

adding a lambda version over one range as well (like the original had)

applying the lambda version in buffer, which nicely removes many helper structures.

It's still in namespace experimental (so don't merge yet), I can't put it in namespace geometry because it conflicts with the structure partition. Any suggestions?

I'm now using detail::lambda_partition.
Still to do:

use it in disjoint
use it in is_valid
use it in relate
So it's getting there.

Then we can deprecate partition itself because it is not used inside the library.

In a next Boost version, we can delete it and rename partition_lambda to just partition.

barendgehrels · 2023-10-04T14:02:45Z

gcc 5 now reports:

/home/gehrels/git_public/boost/boost/geometry/algorithms/detail/overlay/get_turns.hpp:524:32: error: request for member ‘bounding_box’ in ‘sec’, which is of non-class type ‘const int’
                geometry::expand(box, sec.bounding_box, strategy);

This is inconvenient and wrong, it is there and correct. And it is only for the second lambda...

Any idea how to circumvent this?

barendgehrels · 2023-10-04T14:35:53Z

which is of non-class type ‘const int’

It confuses the two overloads of partition_lambda... If I use different names, it works. But that is less convenient....
To be continued.

barendgehrels requested review from mloskot, awulkiew and vissarion May 8, 2023 20:37

barendgehrels self-assigned this May 8, 2023

barendgehrels commented May 8, 2023

View reviewed changes

awulkiew reviewed May 9, 2023

View reviewed changes

barendgehrels force-pushed the experimental-quadtree-partition branch 2 times, most recently from 33e1218 to dc9c4cc Compare May 10, 2023 09:07

vissarion approved these changes May 12, 2023

View reviewed changes

barendgehrels force-pushed the experimental-quadtree-partition branch 2 times, most recently from 17ae254 to 7251ca5 Compare May 12, 2023 17:33

barendgehrels added the experimental Experimental PR - might need work or discussion label Jul 2, 2023

barendgehrels force-pushed the experimental-quadtree-partition branch from 7251ca5 to 0b4bfe7 Compare September 29, 2023 12:47

barendgehrels commented Sep 29, 2023

View reviewed changes

[experimental] update buffer and test with partition version with lam…

d789038

…bdas

barendgehrels force-pushed the experimental-quadtree-partition branch from 0b4bfe7 to d789038 Compare September 29, 2023 12:57

barendgehrels added 4 commits October 4, 2023 15:01

rename to partition_lambda, put in detail, and omit include policy

1410088

use partition_lambda for assign_parents

0eee48d

use partition_lambda for get_turns and self_turns

e7f1905

use partition_lambda for overlay pointlike with linear or areal

31e4145

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[experimental] update test-demo with version with lambdas #1146

[experimental] update test-demo with version with lambdas #1146

barendgehrels commented May 8, 2023

barendgehrels May 8, 2023

barendgehrels May 8, 2023

barendgehrels May 8, 2023

awulkiew May 9, 2023

barendgehrels May 9, 2023

awulkiew May 9, 2023 •

edited

Loading

barendgehrels May 10, 2023

barendgehrels May 10, 2023

awulkiew May 9, 2023

barendgehrels May 9, 2023

awulkiew May 9, 2023 •

edited

Loading

barendgehrels May 9, 2023

vissarion left a comment

vissarion May 12, 2023

barendgehrels May 12, 2023

barendgehrels May 12, 2023

vissarion May 12, 2023

barendgehrels May 12, 2023

barendgehrels commented Sep 29, 2023

barendgehrels Sep 29, 2023

barendgehrels Oct 4, 2023

barendgehrels Sep 29, 2023

barendgehrels commented Oct 4, 2023

barendgehrels commented Oct 4, 2023

barendgehrels commented Oct 4, 2023

		@@ -0,0 +1,17 @@
		# Boost.Geometry (aka GGL, Generic Geometry Library)
		# Robustness Test - convex_hull

[experimental] update test-demo with version with lambdas #1146

Are you sure you want to change the base?

[experimental] update test-demo with version with lambdas #1146

Conversation

barendgehrels commented May 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew May 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vissarion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barendgehrels commented Oct 4, 2023

barendgehrels commented Oct 4, 2023

barendgehrels commented Oct 4, 2023

awulkiew May 9, 2023 •

edited

Loading

awulkiew May 9, 2023 •

edited

Loading