Implement Transitive Paths using binary search instead of a hash map #1313
Conversation
- Renamed nrows, ncols and nvals - Moved code to cpp file - Nullptr is now valid for GrbMatrix::matrix_
- Renamed copy() -> clone() - Functions that create a GrbMatrix now use the unique_ptr directly
- Remove nextIndex_ - Made internal datastructures private - isContained -> contains - const Id& -> Id
- Remove C style arrays - Add checkCancellation
Removed unnecessary unique_ptr and make_optionals
| auto range = std::ranges::equal_range(startIds_, node); | ||
|
|
||
| auto startIndex = std::distance(startIds_.begin(), range.begin()); | ||
|
|
||
| return targetIds_.subspan(startIndex, range.size()); |
There was a problem hiding this comment.
| auto range = std::ranges::equal_range(startIds_, node); | |
| auto startIndex = std::distance(startIds_.begin(), range.begin()); | |
| return targetIds_.subspan(startIndex, range.size()); | |
| return std::ranges::equal_range(startIds_, node); |
Should work, as everything is templated, you don't need to know the type, and it just needs to be iterable.
There was a problem hiding this comment.
No, this does not work. The result of the code you provided are the start ids, which match the node and not the target ids.
Example:
startIds_: 1, 1, 2
targetIds_: 2, 3, 4
equal_range(startIds_, 1) returns {1, 1}, but the correct result would be {2, 3}.
Another option would be to write a wrapper for std::pair<Id, Id> with a custom comparison function. And it would be necessary to convert the two std::span<Id> into a single std::span<std::pair<Id, Id>>. But I would argue that this solution is unnecessarily complex, considering the same effect can be achieved with three lines of code.
Accidentiall fixed too many formats This reverts commit 0d6977f.
src/engine/TransitivePathHashMap.h
Outdated
| auto successors(const Id node) const { | ||
| auto iterator = map_.find(node); | ||
| if (iterator == map_.end()) { | ||
| return std::vector<Id>(); | ||
| } | ||
| std::vector<Id> result(iterator->second.begin(), iterator->second.end()); | ||
| return result; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Don't copy to a vector, but return a const Set&.
You can make an empty set another member of the HashMapWrapper, so that you have something to return in the case of not found.
There was a problem hiding this comment.
I solved this, but I was not sure where to get the ad_utility::AllocatorWithLimit<Id>. There may be a smarter solution.
|
joka921
changed the title
The previous implementation of transitive paths first converted the subtree (the graph which is traversed) into a
HashMap<HashSet<Id>>that represented the adjacency list of the graph. This took a lot of time for large graphs even if the actual transitive path computation was cheap because only a few starting nodes were considered.We now use the sorted subtree as a graph directly using binary search. This is much faster especially since the subtree is often already presorted as it is a single
IndexScan.Currently both implementations are present in the Code. The active implementation can be switched by using a runtime parameter. The old implementation is kept around mostly for benchmarking purposes, as the new implementation should always be cheaper.