Define CS-specific behavior of algorithms only with strategy (part 1). #533

awulkiew · 2018-11-29T17:52:57Z

In Boost.Geometry if a strategy is passed into the algorithm the coordinate system of geometries are ignored. The CS-specific behavior of a strategy is used. At least this is the case at the "highest-level". Internally still in many places the information about the CS defined by strategy is not propagated, default strategies are created from geometry types and algorithms dispatched by CS in algorithms instead of in strategies. To fix this:

proper strategies has to be "derived" from "higher-level" strategy and propagated down to each underlying algorithm,
no default strategy should be created internally,
to be in line with the library design all CS specific code (dispatched by CS) should be moved to strategies directory.

This PR is the first part of changes addressing the above problems. I decided to propose it because it is big enough already and I want to know if the direction I'm moving in is correct.

In order to find places where change is needed semi-automatically I added new coordinate system cs::undefined for which default strategy is not defined nor any algorithm dispatched (besides places where some CS is assumed by default which has to be found manually). Next I modified the following:

move CS-specific normalization utility to strategies namespace strategy::normalize
move CS-specific parts of expand and envelope to strategies, replacing algorithm dispatched by CS with proper strategies,
make equals/disjoint for P/P clean, i.e. strategy now contains CS-specific code and algorithm calls this strategy (internal utility equals_point_point and disjoint_point_point requires strategy now). Also create CS-specific ones. Keep the agnostic one using one of the CS specific default strategies based on geometries.
move CS-specific parts of disjoint for Box/Box into strategies, create proper strategy etc.
move CS-specific parts of disjoint for Segment/Box into strategies.
move CS-specific parts of within Point/Box strategy from agnostic strategies to CS-specific, keep agnostic strategy using default CS-specific strategy based on CS defined by geometries.
point_on_border Midpoint parameter is removed because it was cartesian-only and only was hiding problems (if there was a problem in algorithm and Midpoint used then one could have create rings touching at midpoint and get the same result as in the case of rings touching at endpoint but without Midpoint)
wkt now internally creates default disjoint strategy, I was reluctant to also add Strategy argument to wkt because I thought that stream manipulators should have only 1 argument but dsv has more. Still passing disjoint strategy to wkt feels wierd.
azimuth, normalize, expand, envelope, disjoint, etc. strategies are derived from "higher-level" strategies and passed internally.
overlapping interiors condition fixed in is_valid.
simplify within concept check parameters

The removal of Midpoint in point_on_border exposed a bug in is_valid erroreously returning a false-positive reported here: #515. So at this point several sym_difference tests fail, which is correct.

This work is also why I was postponing fixing #466. I'm going bottom-up with the changes and only now approaching the level of envelope for Polygon. Fixing this issue requires to have a different algorithm for cartesian and non-cartesian Rings and Single-Geometries contained in Multi-Geometries. So at least parts of the algorithm would have to be moved to strategies.

Remove is_radian and units typedef from non-cartesian cs structs.

…tegy step.

…o strategies.

…expand to strategies.

…equals to strategies.

- use CS-specific normalization strategies instead of algorithm. - add lower-level strategies getters (point in point, envelope, expand). - pass strategy to equals_point_point()

…artesian-only hack. If using this parameter changes the result it means that a different method should be used. Using it does not solve the real problem, it only hides it. Consider a polygon in another, touching at the first vertex. Checking the midpoint of the first segment of contained polygon could result in finding out that the polygon is inside. However if the segment was collinear to the containing polygon's segment or this polygon had vertex exactly at the checked midpoint the result would be the same as using the first point of the contained polygon.

- use equals P/P in equals_point_point extracted from strategy passed to algorithm - remove Midpoint of point_on_border (this exposes an error in is_valid) - use envelope and expand strategies extracted from strategy passed to algorithm - change union's default strategy (relate v.s. intersection).

…on_border.

…order with relate).

…trategies and use these strategies in algorithms.

Shadowing of template parameters, missing typename keywords, missing includes.

…om algorithms details to strategies. This fixes circular dependencies.

…his also makes template keywords unnecessary.

…egies. Change P/B within strategy concept (no struct template parameters).

…nits<>::type.

…tegies into algorithms.

barendgehrels

Thanks a lot. I went to most of it and it looks consistent and a lot cleaner. Looks like a lot of code could be deleted.
I'm OK with merging. We both touched a little bit of the same areas so I don't hope that there are too many conflicts. As far as I could judge, mostly the touched lines are different so I think there are not many.

But... is it OK if I merge first?

barendgehrels · 2018-12-12T09:08:23Z

include/boost/geometry/algorithms/detail/disjoint/multipoint_geometry.hpp

@@ -310,23 +317,27 @@ class multi_point_multi_geometry
        }
    };

+    template <typename DisjointPointBoxStrategy>
    struct overlaps_box_point
    {
        template <typename Box, typename Point>
        static inline bool apply(Box const& box, Point const& point)
        {
            // The default strategy is enough for Point/Box


Maybe this comment can be removed now (is it still the default strategy? It can now be specified)

Yes, it should have been removed.

barendgehrels · 2018-12-12T09:11:46Z

include/boost/geometry/algorithms/detail/disjoint/segment_box.hpp

@@ -134,7 +148,8 @@ struct disjoint_segment_box_sphere_or_spheroid

        if (lon1 > lon2)
        {
-            swap(lon1, lat1, lon2, lat2);
+            std::swap(lon1, lon2);
+            std::swap(lat1, lat2);


This is more clear indeed

barendgehrels · 2018-12-12T09:14:41Z

include/boost/geometry/algorithms/detail/envelope/range.hpp

            }
        }
    }

    template <typename Range, typename Box, typename Strategy>
    static inline void apply(Range const& range, Box& mbr, Strategy const& strategy)
    {
-        return apply(boost::begin(range), boost::end(range), mbr, strategy);
+        return apply(Strategy::begin(range), Strategy::end(range), mbr, strategy);


This is surprising. What is this for kind of strategy? So it should implement range like features?

The problem with envelope is that in cartesian we can process points but in spherical and geographic we have to analyse segments because geodesic's vertex may have different latitude (higher or lower on northern or southern hemisphere respectively) than the endpoints. So the strategy feeds the algorithm either with points or with segments. Previously the dispatching by CS was done in the algorithm. I decided to do it this way to allow the strategy to define the smallest Cs-specific portions of the algorithm. Otherwise I'd have to implement the whole calculation of envelope of a range as strategy.

barendgehrels · 2018-12-12T09:22:32Z

include/boost/geometry/io/wkt/read.hpp

@@ -306,6 +306,17 @@ struct stateful_range_appender<Geometry, open>
    }

 private:
+    static inline bool disjoint(point_type const& p1, point_type const& p2)
+    {
+        // TODO: pass strategy


OK for me, PR is large enough

Yes, I wanted to postpone it because I didn't want to force the user to pass within point strategy into read_wkt(). We should probably have separate wkt strategy for that.

barendgehrels · 2018-12-12T09:26:23Z

include/boost/geometry/core/cs.hpp

 {
    typedef geometry::degree units;
 };

 template <>
-struct coordinate_system_units<geometry::radian>
+struct define_angular_units<geometry::radian>


Right, I see. We don't get backward compatibility issues with this?

OK, I see that it is part of core_detail - then it should be no problem

barendgehrels · 2018-12-12T09:28:23Z

include/boost/geometry/core/cs.hpp

-        >::units units;
-};
+    : core_detail::define_angular_units<DegreeOrRadian>
+{};


What is the reason to derive instead of defining it inside?

Less lines of code. AFAIU it's a typical way of defining MPL-like meta-functions.

barendgehrels · 2018-12-12T09:28:56Z

include/boost/geometry/core/cs.hpp

+\brief Undefined coordinate system
+\ingroup cs
+*/
+struct undefined {};


barendgehrels · 2018-12-12T09:32:01Z

include/boost/geometry/algorithms/detail/overlay/get_turns.hpp

+        typename Iterator, typename RangeIterator,
+        typename Section,
+        typename IntersectionStrategy, typename RobustPolicy
+    >
    static inline void advance_to_non_duplicate_next(Iterator& next,


I moved this. So this will give a merge conflict later.

No problem, I'll resolve them.

awulkiew · 2018-12-13T02:12:48Z

No problem with merging your PR first.

vissarion

Thanks Adam, it makes more sense now and much clearer. I reviewed it and added some minor comments. In general I am ok with merging and looking forward to part II.

Additionally, since it seems the right thread to ask: if strategies define the CS-specific computation of an algorithm what is an agnostic strategy? Should just be part of the corresponding algorithm?

include/boost/geometry/algorithms/area.hpp

include/boost/geometry/algorithms/detail/disjoint/segment_box.hpp

include/boost/geometry/algorithms/detail/envelope/interface.hpp

include/boost/geometry/algorithms/detail/envelope/range.hpp

vissarion · 2018-12-13T14:05:08Z

include/boost/geometry/algorithms/detail/envelope/range.hpp

            }
        }
    }

    template <typename Range, typename Box, typename Strategy>
    static inline void apply(Range const& range, Box& mbr, Strategy const& strategy)
    {
-        return apply(boost::begin(range), boost::end(range), mbr, strategy);
+        return apply(Strategy::begin(range), Strategy::end(range), mbr, strategy);


vissarion · 2018-12-13T14:10:07Z

include/boost/geometry/algorithms/detail/equals/point_point.hpp

 {
-    return ! detail::disjoint::disjoint_point_point(point1, point2);
+    return Strategy::apply(point1, point2);


Why we need a special strategy for checking if 2 points are equal? Is it the same for all CS? Unless if you consider normalization, but in this case how normalization strategy is passes here? What am I missing...

In general yes, this could be done the same for all coordinate systems. For this we'd have to agree what's the valid range of coordinates, e.g. if lon_deg = -180+eps/2 is valid, since this would be true: bg::math::equals(lon_deg + 360, 180), this coordinate would be ambiguous and cartesian-like comparison would fail. Currently we have this normalization in place which even if the user represents the coordinates ambiguously (-180 vs 180 or even as something outside valid range) the relation operations still works. In order for this to work we need separate strategies for various CSes even for points.

I do not have a strong opinion here, since I cannot predict the user needs, but what about fixing a range e.g. (-180,180], then if the user compare invalid coordinates the result is "unexpected" as with invalid geometries. In any case the user could be use normalization before comparing and have an always correct result.

In any case, this should be done in a new PR.

Yes, the normalization could also be a part of correct. However I think that assuming that -180 is correct is common practice and if we changed the existing behavior users could get unexpected results.

include/boost/geometry/algorithms/detail/expand/interface.hpp

vissarion · 2018-12-13T14:28:45Z

include/boost/geometry/algorithms/detail/overlay/get_turns.hpp

@@ -482,11 +492,13 @@ class get_turns_generic

        typename IntersectionStrategy::envelope_strategy_type const
            envelope_strategy = intersection_strategy.get_envelope_strategy();


is it needed for intersection strategy to contain both envelope and expand strategies? Why not passing them independently as in the strategies in disjoint_segment_box?

The sectionalize uses 2 policies. And even if we wanted to calculate boxes and expand spherical boxes by other boxes instead of segments, spherical boxes are expanded in different way than cartesian ones so we'd also need second strategy here (expand box/box).

vissarion · 2018-12-13T14:30:52Z

include/boost/geometry/algorithms/detail/within/interface.hpp

@@ -70,13 +70,7 @@ struct within
                             Geometry2 const& geometry2,


same here. why no dispatching as in area and distance?

I answered that before but also here, dispatching based on function templates doesn't force us to explicitly state the types. I've done it there only because I was forced to do it, not because I wanted to do it. ;)

vissarion · 2018-12-13T14:41:19Z

test/strategies/segment_envelope.cpp

@@ -1,6 +1,6 @@
 // Boost.Geometry


now there is strategy named envelope_segment the name of this file should also change

awulkiew · 2018-12-16T14:34:49Z

Merged develop with recently merged #531 and #540.

As a reminder, note that some sym_difference tests fails due to this: #515

Additionally, since it seems the right thread to ask: if strategies define the CS-specific computation of an algorithm what is an agnostic strategy? Should just be part of the corresponding algorithm?

@vissarion I'm not sure. Maybe it indeed should simply be a different algorithm with distinguished name, like bg::algorithm_variant1, bg::algorithm_variant2, bg::algorithm_variant3 and the default bg::algorithm. In that case I'm not sure if the name "Strategy" is a good name since it's more general, maybe it would then be "CSStrategy". But anyway, I'm not sure if we can change that now because of causing problems to the users. Thought If there is only 1 agnostic strategy for algorithm I doubt anyone is using it explicitly. Maybe besides buffer's strategies which are probably used heavily because they are something between Strategies and Parameters. And on top of this we might decide to move to C++11 like some other libraries and this could be a good opportunity to redefine some aspects of the library. IF we want to talk about all of it I suggest to create an Issue.

…t strategy definition.

awulkiew added 30 commits October 12, 2018 02:27

[core][geometries][srs] Add cs::undefined and detail::cs_angular_units.

0be273f

Remove is_radian and units typedef from non-cartesian cs structs.

[area][distance] Dispatch on struct level by strategy at resolve_stra…

9611515

…tegy step.

[formulas] Fix coordinates comparison warning.

5b6651d

[normalize][strategies] Move CS-specific part of normalization code t…

94c7c81

…o strategies.

[envelope][expand][strategies] Move CS-specific code of envelope and …

17017e4

…expand to strategies.

[disjoint][equals][strategies] Move CS-specific code of P/P disjoint/…

2a93705

…equals to strategies.

[strategies] Update WRT recent changes:

64863cc

- use CS-specific normalization strategies instead of algorithm. - add lower-level strategies getters (point in point, envelope, expand). - pass strategy to equals_point_point()

[io] In wkt read/write use default P/P equals strategy.

c7e8540

[test][envelope][strategies] Update strategies used for testing.

dcaadf6

[test][is_valid] Add test case failing with Midpoint passed to point_…

9ebdda9

…on_border.

Merge branch 'develop' into feature/undefined_cs

38cbc16

[algorithms] Fix copyright date.

5847581

[is_valid] Fix overlapping interiors condition (replace 2x point_on_b…

fbe8f51

…order with relate).

[test][is_valid] Add test failing with midpoint in point_on_border.

c15b329

Merge branch 'develop' into feature/undefined_cs

ef432a7

Merge branch 'develop' into feature/undefined_cs

d29acd1

[algorithms][strategies] Move disjoint Box/Box CS- specific code to s…

4ac1f45

…trategies and use these strategies in algorithms.

[test][partition] Use disjoint Box/Box strategy in partition test.

bcd6e69

[algorithms][strategies] Fix several compilation errors.

7d45cae

Shadowing of template parameters, missing typename keywords, missing includes.

[algorithms][strategies] Move CS-specific code of envelope_segment fr…

7af8085

…om algorithms details to strategies. This fixes circular dependencies.

[algorithms] Add missing typename keywords.

7dc81a9

[index] In intersection_content() explicitly use disjoint B/B strategy.

fe63e8e

[is_valid] Fix unused variable warning.

99293c8

[strategies][envelope] Simplify envelope::cartesian_point strategy. T…

80ffb22

…his also makes template keywords unnecessary.

[is_valid] Fix unused variable warning.

12ff7a6

[algorithms][strategies] Implement separate cart/sph P/B within strat…

6c879b7

…egies. Change P/B within strategy concept (no struct template parameters).

[test][within] Update P/B within strategies.

a0eb54f

[formulas] Replace coordinate_system<>::type::units with cs_angular_u…

3756919

…nits<>::type.

awulkiew added 3 commits October 15, 2018 01:26

[algorithms][strategies] Propagate disjoint(P,B) and expand(B,B) stra…

4d2c60d

…tegies into algorithms.

Merge branch 'develop' into feature/undefined_cs

d192548

Merge branch 'develop' into feature/undefined_cs

fc34634

awulkiew added the enhancement label Nov 29, 2018

awulkiew added this to the 1.70 milestone Nov 29, 2018

barendgehrels approved these changes Dec 12, 2018

View reviewed changes

vissarion reviewed Dec 13, 2018

View reviewed changes

awulkiew added 2 commits December 16, 2018 15:16

Merge branch 'develop' into feature/undefined_cs

4c099bd

[get_turns] Remove unneeded comment.

371767b

[test][strategies] Rename envelope_segment test file.

f6826c4

awulkiew merged commit c7ba195 into boostorg:develop Dec 16, 2018

awulkiew referenced this pull request Jan 23, 2019

[index] Fix compilation error in intersection_content(): wrong defaul…

247f657

…t strategy definition.

awulkiew mentioned this pull request Jan 24, 2019

Fix or suppress validity test failures in set operations #550

Merged

awulkiew mentioned this pull request Mar 21, 2019

disjoint unexpectedly returns true #466

Closed

awulkiew mentioned this pull request Jun 24, 2019

Define CS-specific behavior of algorithms only with strategy (part 2). #605

Merged

awulkiew mentioned this pull request May 6, 2020

[buffer] remove rescaling from the turn-in-original check #708

Merged

awulkiew mentioned this pull request Jun 3, 2020

Umbrella strategies proposal. #717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define CS-specific behavior of algorithms only with strategy (part 1). #533

Define CS-specific behavior of algorithms only with strategy (part 1). #533

awulkiew commented Nov 29, 2018

barendgehrels left a comment

barendgehrels Dec 12, 2018

awulkiew Dec 13, 2018

barendgehrels Dec 12, 2018

barendgehrels Dec 12, 2018

awulkiew Dec 13, 2018 •

edited

Loading

vissarion Dec 13, 2018

barendgehrels Dec 12, 2018

awulkiew Dec 13, 2018

barendgehrels Dec 12, 2018

barendgehrels Dec 12, 2018

awulkiew Dec 13, 2018

barendgehrels Dec 12, 2018

barendgehrels Dec 12, 2018

awulkiew Dec 13, 2018

awulkiew commented Dec 13, 2018

vissarion left a comment •

edited

Loading

vissarion Dec 13, 2018

vissarion Dec 13, 2018

awulkiew Dec 13, 2018 •

edited

Loading

vissarion Dec 14, 2018 •

edited

Loading

vissarion Dec 14, 2018

awulkiew Dec 14, 2018

vissarion Dec 13, 2018

awulkiew Dec 13, 2018 •

edited

Loading

vissarion Dec 13, 2018

awulkiew Dec 16, 2018

vissarion Dec 13, 2018

awulkiew Dec 16, 2018

awulkiew commented Dec 16, 2018

		@@ -482,11 +492,13 @@ class get_turns_generic

		typename IntersectionStrategy::envelope_strategy_type const
		envelope_strategy = intersection_strategy.get_envelope_strategy();

Define CS-specific behavior of algorithms only with strategy (part 1). #533

Define CS-specific behavior of algorithms only with strategy (part 1). #533

Conversation

awulkiew commented Nov 29, 2018

barendgehrels 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

awulkiew Dec 13, 2018 • 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

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 commented Dec 13, 2018

vissarion left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awulkiew Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

vissarion Dec 14, 2018 • 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

awulkiew Dec 13, 2018 • 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 commented Dec 16, 2018

awulkiew Dec 13, 2018 •

edited

Loading

vissarion left a comment •

edited

Loading

awulkiew Dec 13, 2018 •

edited

Loading

vissarion Dec 14, 2018 •

edited

Loading

awulkiew Dec 13, 2018 •

edited

Loading