Closed B4rtDC closed 3 years ago
Yes, you are complete right! I already pushed a fix for this in #70. I would still like to run a few tests to quantify the effect on runtime and the resulting optimisation before I merge it. Feel free to take it already for a test!
If you find anything else, do let me know!
Thanks for the quick reply. I missed https://github.com/vtraag/leidenalg/pull/70#issue-627208312 that one.
I'll keep you posted should I come across something else :-)
Hi,
first of all, congratulations on this great package! While going over some details of the code, I came across a problem in the segment below :
https://github.com/vtraag/leidenalg/blob/e47a1881721b4e5ca8cf056ea74a884ebee2aaba/src/leidenalg/Optimiser.cpp#L728-L739
If you wan't to realise what comment (731-733) says, shouldn't line 734 be the following?
Before moving or if you don't move v,
membership(v)
equalsmax_comm
https://github.com/vtraag/leidenalg/blob/e47a1881721b4e5ca8cf056ea74a884ebee2aaba/src/leidenalg/Optimiser.cpp#L569 https://github.com/vtraag/leidenalg/blob/e47a1881721b4e5ca8cf056ea74a884ebee2aaba/src/leidenalg/Optimiser.cpp#L651 This leads tomembership(v) != max_comm
being false so the neighbor u won't be placed in the queue.If you move the community of v, you call upon the
move_node(v, max_comm)
function, which changes the assignment of v tomax_comm
. This also leads tomembership(v) != max_comm
being false, with the same result.In both cases, I'm not sure that the node u will ever be placed in the queue as intended. If this is indeed an error, it might also be present in the other functions that work in a similar way.
However, if all of the above is fine, could you explain what is meant with the comment?
Kind regards