Implement Kruskal traversal iterator#336
Implement Kruskal traversal iterator#336ameligrana wants to merge 2 commits intoJuliaGraphs:masterfrom
Conversation
Co-authored-by: kylebeggs <beggskw@gmail.com>
| mutable struct KruskalIteratorState <: AbstractIteratorState | ||
| edge_id::Int | ||
| mst_len::Int | ||
| end |
There was a problem hiding this comment.
What is the advantage of having a mutable iterator state here? Couldn't we simply return a new state?
| end | ||
| end | ||
|
|
||
| Base.length(t::KruskalIterator) = nv(t.graph)-1 |
There was a problem hiding this comment.
Is that true in case the graph is not connected?
There was a problem hiding this comment.
I think you are completely correct, but I don't have time to work on this at the moment, instead I would be willing to finish up #163, if you happen to have some review time I think it could be better to redirect it there :-)
(by the way I think also the other PR suffer from the same problem)
|
To be honest, I am not sure if an iterator is of much use here. We seem to do most of the allocations when create the iterator and we also do not seem to free any memory after each iteration. But perhaps I am wrong here. |
|
This has stalled since the last round of review. For now I will mark it as a draft, to keep my review queue a bit more organized, but do not hesitate to convert it to not a draft if you have the energy to continue this submission. |
|
forgot to note: this would also need to be tested for correctness against the related existing code |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #336 +/- ##
==========================================
- Coverage 97.22% 96.94% -0.29%
==========================================
Files 122 123 +1
Lines 7240 7261 +21
==========================================
Hits 7039 7039
- Misses 201 222 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4 participants
part of #163, to be revisited after that pr is merged