Geometry Boost List: Re: [geometry] Run-Time specified geometries

Subject: Re: [geometry] Run-Time specified geometries
From: Adam Wulkiewicz (adam.wulkiewicz_at_[hidden])
Date: 2014-04-25 11:17:54

Hi,

Samuel Debionne wrote:
> Hello Adam,
>
>> The best solution would be the implementation of all algorithms for all
>> Geometries. But this is the theory...
> For binary algorithms, not every combination of types are relevant. For
> instance the equals algorithm has a very limited set of supported
> geometries combination :
>
> http://www.boost.org/doc/libs/1_55_0/libs/geometry/doc/html/geometry/reference/algorithms/equals.html

Yes, we plan to release more combinations with Boost 1.56, still 
equals() will never be true for e.g. Linestring/Polygon but for such 
cases false could be returned. Probably also for cases where OGC 
standard explicitly says that some relation is not defined e.g. 
crosses() for Areal/Areal or touches() for Pointlike/Pointlike. But 
maybe the exception should be thrown for run-time geometries. In short, 
I get your point.

>>> template <typename Geometry1, typename Geometry2>
>>> struct not_implemented
>>> {
>>>       typedef typename boost::mpl::or_<
>>>           typename boost::is_base_of<
>>>               nyi::not_implemented_tag,
>>>               dispatch::distance<Geometry1, Geometry2>
>>>           >::type,
>>>           typename boost::mpl::not_<
>>>               typename boost::is_same<
>>>                   typename dimension<Geometry1>::type,
>>>                   typename dimension<Geometry2>::type
>>>               >::type
>>>           >::type
>>>       >::type
>>>       type;
>>> };
> For instance, the not_implemented metafunction above try to evaluate the
> availability of an implementation given the parameters type of the
> distance algorithm. The result is used to avoid the call to distance and
> trigger a runtime exception instead. Since the algorithm is no
> instantiated, there is no compile time error.
>>> On a side note that also requires defining the
>>> BOOST_GEOMETRY_IMPLEMENTATION_STATUS_BUILD macro to workaround an MPL
>>> assertion within the not_implemented_error class.
>> Ah, yes this macro is there for the purpose of disabling MPL_ASSERT for
>> automatic check of the supported Geometries (basically what you're
>> doing) for docs. So probably shouldn't be used to disable "normal" error
>> handling. At least for now.
> I think it could be used not as originally intended but also to relax
> compile time error reports to better support runtime geometries.

Hmm, maybe we could move MPL_ASSERT from the not_implemented<> members 
to some default implementations of not_implemented<>::apply(). The 
drawback is that it should be some number of those default apply() 
methods to handle all possible number of parameters. It should fail only 
when the user wanted to call the function but it would also be possible 
to check if the algorithm is implemented in compile-time without raising 
the alarm. Something like this:

template <typename ReturnType, ...>
struct not_implemented
     : nyi::not_implemented_tag
{
     template <typename P1>
     static inline ReturnType apply(P1 &)
     {
nyi::not_implemented_error<...> err;
         ReturnType * r = NULL
         return *r;             // RATHER HACKY
     }

     template <typename P1, typename P2>
     static inline ReturnType apply(P1 &, P2 &)
     {
         //...
     }

     // etc.
};

or something more elegant, because I fear that some static code analyser 
may complain about those pointers.

Or was the current design choosen not only for convenience but also for 
some other, specific reason than I cannot see?

>
>> AFAIU, if an algorithm is not implemented for some pair of Geometries
>> the dispatch::xxx should be derived from not_implemented<>. And each
>> dispatched implementation should just use whatever works. So you should
>> be able to rely on this. Otherwise it should be fixed.
> That would be awesome to have the not_implemented class as the sole way
> to report unimplemented feature or bad parameter types ! Unfortunately
> there are other places where MPL_ASSERT is called to report bad
> parameter type (e.g. preconditions such as the checking if the
> geometries have the same dimensions)

This is the intention of having this not_implemented<> class. I realize 
that there are places where some shortcuts been made and 
not_implemented<> wasn't used. Those places should probably be fixed.
AFAIR strategies should also be derived from not_implemented<> so this 
could be checked the same way.

I think you shouldn't be forced to think about all possible details that 
may not compile. If the algorithm and strategy are implemented for 
passed Geometries and Geometries are correct (Concept checks are for 
this purpose), everything should compile. Otherwise it's probably a bug.

Regarding the compatibility of Geometries, it could be done on the 
dispatch::xxx level. Where are those explicitly dimensions compared?

>
>> Btw, I couldn't find the extreme_points in intersects implementation.
>> Where did you find it?
> It's not a direct call. The approximative call stack is :
>
> intersects -> disjoint -> point_on_surface -> extreme_points

It should be used only in implementations for Areal geometries. As said, 
otherwise it's a bug.

>
>>> 4. Maybe an extension is not a good idea and runtime support should be a
>>> core functionality of the library. I think so because it seems to
>>> require an extra requirements : the algorithms must report compile time
>>> errors in a specified way when invoked with not supported types not just
>>> rise an MPL assertion.
>> So assuming we decided that the implementation status should be reported
>> in run-time for Variants...
> My suggestion is that both implementations co-exist : the actual
> implementation and a more relax implementation (possibly an extension)
> that would fire runtime exception.
>
>> Yes, basically dispatch of the algorithm could e.g. take some
>> NotImplementedPolicy parameter and by default pass it deeper to
>> not_implemented<> or just use it as a base class. Then the standard
>> version of the algorithm could use some compile-time policy and
>> variant-aware version could use run-time one.
> A policy is fine. I'm just a bit worried that that extra parameter will
> spread everywhere.

Well we'd be forced to probably use it (only) in not_implemented<>, 
dispatch::xxx<> and maybe some aditional classes used for reversal of 
parameters and handling of Variants.
But with the approach mentioned above, is this Policy needed?

>
>> The alternative would be to build another dispatching system for the
>> purpose of run-time libraries errors, a mirror of the compile-time
>> dispatching, and put it above/on top. This would complicate the
>> maitnance, a lot of code would be duplicated, etc.
> I think the runtime versions of the algorithms should be in their own
> namespace (e.g. boost::geometry::runtime) and reuse a maximum of the
> existing code.
>
> I have setup a runtime branch on my fork and will try to add some of my
> experiments there to make the discussion less abstract.
>
> https://github.com/sdebionne/geometry/commit/849fd51679f2cfb22354dab561d4f65b82159a40

Thanks for your effort!

Regards,
Adam