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

feat(adjacency) hasVertex for Graphs #400

Merged

Conversation

dearlordylord
Copy link
Contributor

predecessor: #398

I thought a bit about addVertex:

  • for Adjacency Matrix, it seems it doesn't fit the adjacency matrix model since it contradicts its definition; IF we are to store vertices info in it, it'll become not just BitField but BitMatrix (since we need n^2 booleans to keep the number of edges); which effectively becomes BitMatrix

  • for BitMatrix, I haven't added addVertex yet and haven't considered it diligently yet, since it doesn't fit the initial idea of "Grapg Interface" anymore (no "addVertex" because of the AdjacencyList consideration above)

Note that BitField doesn't seem to be enough to track Vertices because it loses the info about "how many are there"

Therefore I'm isolating this PR to hasVertex only:

  • still, there is no BitMatrix (or BitField) used for AdjacencyList because of the considerations above (it seems it becomes AdjacencyBitMatrix, effectively, and we already have that), therefore edge presence implemented through degree

  • for AdjacencyBitMatrix, BitMatrix is already there, however O(n) scans needed to determine the presence of a bit to know a vertice is present

  • nothing new for AdjacencyList

Added some tests, including a bit unrelated bitFIeld test (it is a side effect of me figuring it out 😆 )

@dearlordylord
Copy link
Contributor Author

Initial comment of @postspectacular

#398 (comment)

Thanks, @Firfi - this seems like an easy enough fix, but the main reason that method didn't exist so far is that I wasn't quite clear myself yet how to handle this in the general case of the IGraph interface, which should be the common API for all three implementations provided by this package. For the AdjacencyList impl, the mere presence of an entry for a vertex ID is proof the vertex exists, but for AdjacencyBitMatrix and AdjacencyMatrix this isn't the case and there we can currently only infer if a vertex exists iff there are also any incoming/outgoing edges associated with it. One my ideas to overcome this was adding another bitfield for keeping track of explicitly or implicitly declared vertices and then add addVertex(id) and hasVertex(id) methods to the IGraph interface. The only other thing to consider there is the additional memory overhead (122KB per million vertices), but maybe that's not too bad...

@dearlordylord
Copy link
Contributor Author

@postspectacular please let me know in case I didn't get it right, I may have, in fact

@postspectacular
Copy link
Member

Thank you very much @Firfi - I can only look at this on Tuesday the earliest, but will report back ASAP!

@dearlordylord
Copy link
Contributor Author

@postspectacular bumping this, sorry for ping but quite a lot of time passed already 🥲

@postspectacular
Copy link
Member

Hi @Firfi - so sorry for the delay, this thing completely slipped my mind... will take another look this week!

@postspectacular postspectacular merged commit b2e8851 into thi-ng:develop Oct 19, 2023
@postspectacular
Copy link
Member

Thank you so much once more, @Firfi! It's all merged & released now. It somehow ended up as a patch version only (v2.4.1) but should have been v2.5.0, have to check why, likely because your actual feature commits are from so long ago (mea culpa!)... might do another release, to explicitly force proper version bump. Also added you as contributor

👍

postspectacular added a commit that referenced this pull request Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants