Fix for OOM issue with transitive paths #1621
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1621 +/- ##
==========================================
+ Coverage 89.30% 89.33% +0.02%
==========================================
Files 373 374 +1
Lines 35377 35695 +318
Branches 3999 4051 +52
==========================================
+ Hits 31594 31887 +293
- Misses 2493 2519 +26
+ Partials 1290 1289 -1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
This did not fix it |
hannahbast
changed the title
src/engine/LocalVocab.h
Outdated
| @@ -97,6 +97,9 @@ class LocalVocab { | |||
| void mergeWith(const R& vocabs) { | |||
| auto inserter = std::back_inserter(otherWordSets_); | |||
| for (const auto& vocab : vocabs) { | |||
| if (vocab.empty()) { | |||
There was a problem hiding this comment.
Can usestd::views::filter(std::not_fn(&LocalVocab::empty())
src/engine/LocalVocab.h
Outdated
| @@ -97,6 +97,9 @@ class LocalVocab { | |||
| void mergeWith(const R& vocabs) { | |||
| auto inserter = std::back_inserter(otherWordSets_); | |||
| for (const auto& vocab : vocabs) { | |||
| if (vocab.empty()) { | |||
| continue; | |||
| } | |||
There was a problem hiding this comment.
And you should also refactor the
LocalVocab::clone() method to call this mergeWith method:
... clone() {
LocalVocab clone;
clone.mergeWith(std::span(this, 1));
return clone;
}
| @@ -244,7 +246,7 @@ class TransitivePathImpl : public TransitivePathBase { | |||
| */ | |||
| NodeGenerator transitiveHull(const T& edges, LocalVocab edgesVocab, | |||
| std::ranges::range auto startNodes, | |||
| std::optional<Id> target) const { | |||
| std::optional<Id> target, bool yieldOnce) const { | |||
There was a problem hiding this comment.
Add to the docstring, that the yieldOnce has to have the same value as in the using code, and that it is only used for optimizing the local vocabs.
Conformance check passed ✅No test result changes. |
|
|
Reopened as #1627 |
Checking if this fixes #1606