thi-ng / umbrella

⛱ Broadly scoped ecosystem & mono-repository of 199 TypeScript projects (and ~180 examples) for general purpose, functional, data driven development
https://thi.ng
Apache License 2.0
3.41k stars 151 forks source link

feat(adjacency) hasVertex for Graphs #400

Closed dearlordylord closed 1 year ago

dearlordylord commented 1 year ago

predecessor: https://github.com/thi-ng/umbrella/pull/398

I thought a bit about addVertex:

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:

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

dearlordylord commented 1 year ago

Initial comment of @postspectacular

https://github.com/thi-ng/umbrella/pull/398#issuecomment-1531584361

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 commented 1 year ago

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

postspectacular commented 1 year ago

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

dearlordylord commented 1 year ago

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

postspectacular commented 1 year ago

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

postspectacular commented 1 year ago

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

👍