Replies: 5 comments
-
@awulkiew thanks a lot for the proposal, really good if we go forward with the Umbrella Strategy. I wanted to react earlier already but these things take some time. I like the examples from user perspective. I assume it is still possible to be more specific (for example: specify Haversine) or less specific (pass no strategy at all), such that all most common scenarios don't break. I also like the explicit example that we can specify one strategy for multiple algorithms, which is what we want indeed. About the implementation, I didn't dive into all the details yet - but is it also possible to implement it tag-dispatch-based instead of In the very last example, why do you write |
Beta Was this translation helpful? Give feedback.
-
I also agree and like the design proposal in general. Thanks @awulkiew ! I have some comments regarding the user perspective. For naming I think we should use the same for both algorithm and strategy. For example even in this case
although there is clear naming the user should know or search what is the correct name of the strategy for
At this point I have a question regarding the design, not of this proposal but in general. Where is the right place to store the coordinate system type? Right now this is stored in geometries and used to deduct the default strategy. But if a user defined specific strategy is used then the CS of the geometry is overlooked. But this is not the case for the units of the CS, i.e. the user defined strategy overlook the coordinate system but uses its units (and in case of cartecian CS it treats coordinates as radians). This is to me a bit unclear as a design and also allow the user to do weird things that are not easy to identify e.g.
I think that the minimum would be here to throw a compile time error if the CS of the geometry is different that the CS implied by the strategy. |
Beta Was this translation helpful? Give feedback.
-
Yes, I omitted the default and creation of umbrella strategy from other strategies from the example. I assumed that UmbrellaStrategy would be the default strategy created at the top-most level of the algorithm if no strategy is passed and that there would be a backward-compatibility mechanism of creating umbrella strategy from simple strategy like haversine.
Yes it would be possible to dispatch with tags. Thought it would require to implement more structs and their specializations. AFAIU SFINAE was avoided in the past because older compilers didn't support it properly. With C++14 requirement this shouldn't be a problem.
These algorithms are only examples. I didn't want to obscure the strategies implementation with the algorithms implementation. You may look at them as if they were parts of the algorithm dispatched for geometries, so what is typically implemented in
I agree.
Right, I'm not sure how we should address this issue. Because this compiled since the beginning, because there is no CS information in strategies, because it is possible to get radians from cartesian geometries, etc. I thought that the intention is that the strategy defines the CS because this is the only way to do consistent handling of the CS when you can pass an arbitrary strategy with unknown CS. This is why I expressed it in our earlier discussions and also in this Issue: #694 (so it's possible I was wrong there). And this is why I also proposed the Yes, we could do so the the library fails to compile when incompatible geometry and strategy is passed and if As you noticed units are another issue. For now they are taken from geometry in all cases and by default they are There is also another thing, i.e. distinction between |
Beta Was this translation helpful? Give feedback.
-
I agree. |
Beta Was this translation helpful? Give feedback.
-
That is true. I'm OK if we try it like this, your example looks good.
This was implemented intentionally. It is not uncommon to apply cartesian strategies on geographic coordinates, on small scales you just can do that (you will lose some precision). As Adam said, it was always supported like this. I don't think it is a problem.
Fine for me. |
Beta Was this translation helpful? Give feedback.
-
This is a followup of a discussion about umbrella strategies from this PR: #708 and a comment (from somewhere else?) which I'm unable to find right now.
THE PROBLEM
I'd like to propose an interface of umbrella strategies. I had several goals in mind:
Additional info:
THE SOLUTION
An example of what I'm proposing from user's perspective:
Notice clear naming and also that the scope of these particular umbrella strategies is the whole algorithm, i.e. they can be passed for all combinations of geometries. The design is modular so it is possible to implement an umbrella strategy for only one or several combinations of geometries or to have the same strategy for all algorithms, e.g.:
I think that we should focus on algorithm-scope and cs-scope umbrella strategies and to not bother with the combinations of geometries.
THE IMPLEMENTATION
We have to go one more level of abstraction above what we currently have as strategies. No matter what other/unrelated strategies are there in the umbrella strategy any algorithm should be able to access whatever subset of strategies it needs and pass the umbrella strategies to whatever other lower-level algorithms that are called internally. This is why I propose to have umbrella strategies with clearly-named member functions returning simple strategies (the ones with
apply()
). So any algorithm would call a member function of the same name as the algorithm, get the simple strategy and call it, then if needed pass the umbrella strategy to some other algorithms.Like that (these are combination-specific implementations, so what would typically be in
namespace dispatch
):So with this design it is possible to pass any umbrella strategy containing any subset of simple strategy getters as long as the minimal subset needed by a particular algorithm and all other lower level algorithms is available.
Now, the implementation of strategies. The getters for particular combinations are enabled with
std::enable_if
and for each algorithm there is a convenient base class defining only the simple strategies of a given algorithm. This allows to create compound strategies easily. This probably will have to be replaced by something different in spherical and geographic to avoid storing multiple instances of the earth model in bigger compound strategies. But for the time being, here is how it looks like:So with the above, it is possible to use all of it like that:
What do you think?
Beta Was this translation helpful? Give feedback.
All reactions