Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

Making getNodesInCluster() recursive #3881

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Menighin
Copy link

No description provided.

@Tooa
Copy link
Member

Tooa commented Apr 2, 2018

Hi, thanks for your contribution. Would you please add some word on why a recursive implementation is better for this use case? Have you tested your code and the previous implementation with respect to time and space complexity?

* @returns {Array.<Node.id>}
*/
getNodesInCluster(clusterId) {
getNodesInCluster(clusterId, recursive = false) {
Copy link
Member

Choose a reason for hiding this comment

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

What about adapting the documentation here [1]? Is there any benefit of making the actual algorithm implementation configurable? Is this something the user might care?

[1] http://visjs.org/docs/network/index.html?keywords=getNodesInCluster

@@ -850,15 +850,25 @@ class ClusterEngine {
/**
*
* @param {Cluster.id} clusterId
* @param {Recursive} recursive
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be boolean instead of {Recursive}? I would expect something like @param {boolean} [recursive=false]

Correct me if I'm wrong.

@Menighin
Copy link
Author

Menighin commented Apr 2, 2018

Hi @Tooa , I will make the changes requested asap.
Meanwhile about your questions, I have not make any specific stress test on this approach.

Currently I am working on what I consider to be a medium-size level-clustered network: It starts with 3 clustered that contains another clustered nodes that contains another clustered nodes ...... until it opens the "leaf" nodes.

The recursive method comes in hand when the user clicks a clustered node that contains another clustered node. In my case, when this interaction is made, I would like to get all the leaf nodes the clicked clustered node contains. The current implementation is only returning me the clustered nodes inside the clicked clustered node.

I'm not sure if I understood right your questions, if there's any doubt left please feel free to ask!

Regards,

Edit: Ah! Please note that when the recursive parameter is false, the implementation is exactly the same it was before!

@micahstubbs
Copy link

@Menighin thanks for the sending this PR with the fix!

Curious, could you provide a new example that we can use to verify the fix, or point us to an existing example that shows the bug behavior?

https://github.com/almende/vis/tree/master/examples/network

said another way, could you provide steps to test this PR?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants