Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Kruskal traversal iterator #336

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Tortar
Copy link
Contributor

@Tortar Tortar commented Jan 30, 2024

part of #163, to be revisited after that pr is merged

Comment on lines +30 to +33
mutable struct KruskalIteratorState <: AbstractIteratorState
edge_id::Int
mst_len::Int
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that true in case the graph is not connected?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@simonschoelly
Copy link
Member

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.

@gdalle gdalle added the enhancement New feature or request label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants