Add Aggregate method#33
Conversation
| { | ||
| if (canceled) | ||
| { | ||
| return nullptr; |
There was a problem hiding this comment.
Good catch! I am thinking of the classic reduce functions, that often get a "null" object to start from. This could be nullptr as well as 0 or any other object that is of type T.
There was a problem hiding this comment.
Would
T ret = {};
return ret;
work? I ask because there's really no such thing as an initial value, since lots of leaf nodes will potentially be merged with the "null" object.
| /// \param combine a function to combine two aggregation types | ||
| /// \param progress a function to check if the aggregation should continue | ||
| template <typename T> | ||
| T Aggregate(std::function<T(const DATATYPE &a1)> convert, std::function<T(T &a1, T &a2)> combine, std::function<bool()> progress); |
There was a problem hiding this comment.
This function looks like an excessive responsibility to this class. Could you consider placing it to an independent function leveraging such functional through visitor pattern?
But it is up to maintainer to decide, I'm just a window-shopper here 😛 .
There was a problem hiding this comment.
I'm open to any changes that get this functionality into the library in a way that still works for my use case. I'm only vaguely familiar with the visitor pattern and don't immediately see how to switch the implementation.
There was a problem hiding this comment.
Actually, thinking back, the reason I didn't just walk the tree myself and do what I wanted with the nodes is that they weren't public. If folks are OK with exposing the internals of the tree, my problems go away.
There was a problem hiding this comment.
@adv12 what do you need the protected nodes actually for? Consider the following approach btw where you do introduce independent function to the library and not increasing its coupling value:
auto convert = [](...){...};
auto combine = [](...){...};
auto progress = [](...){...}
template<typename T>
T Aggregate(const RTree &tree)
{
T combinedValue;
RTree::Iterator it;
tree.GetFirst(it);
auto *prevValue = tree.GetAt(it);
combinedValue = convert(*prevValue);
tree.GetNext(it);
// check it for presense of value;
// do the iteration further:
for(tree.GetNext(it); !tree.IsNull(it); tree.GetNext(it))
{
auto *value = tree.GetAt(it);
auto converted = convert(*value);
combinedValue = combine(combinedValue, converted);
}
return combinedValue;
}
Read it as pseudocode rather :D
There was a problem hiding this comment.
As far as I understood the article on the algorithm, just iterating over the nodes, not regarding the bottom-up-approach, the runtime is very very long.
It turns out that Cascaded Union can be much more efficient than Iterated Union. On a test dataset with 30,000 small polygons with a high degree of overlap, the performance results were:
Cascaded Union: 20 sec
Iterated Union: 3 hours 40 min !
There was a problem hiding this comment.
Yes, the iteration over the leaves is the least optimized portion of the algorithm. But the main point is that by aggregating from the bottom up, you're progressively reducing the remaining work at higher levels.
There was a problem hiding this comment.
Oh, and @drewpts, if it's not obvious why, take a look at the article linked in the PR. At each level, you're combining the simplified geometries from the level below into an even more simple geometry. The speedup isn't due to the iteration order, per se, but the progressive simplification of the problem from the bottom up.
| /// \param combine a function to combine two aggregation types | ||
| /// \param progress a function to check if the aggregation should continue | ||
| template <typename T> | ||
| T Aggregate(std::function<T(const DATATYPE &a1)> convert, std::function<T(T &a1, T &a2)> combine, std::function<bool()> progress); |
There was a problem hiding this comment.
Could you please provide an example for the progress function? I imagined it depends on the current data to determine if to continue, but it does not get any parameters. Is it used to halt the execution from another thread?
There was a problem hiding this comment.
Halting execution from another thread was the need for my application. I could make it accept the current data if that would be more generally useful.

I needed to be able to aggregate nodes in the tree from the bottom up to implement cascaded polygon union using this library, so I added an Aggregate method. It takes three functions as parameters: one to convert the stored data type to the aggregate type, one to aggregate two items of the aggregated type, and a third to check for cancellation.