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

Can't include hidden nodes to cluster in joinCondition #3164

Closed
dheerajbapat opened this issue Jun 12, 2017 · 9 comments
Closed

Can't include hidden nodes to cluster in joinCondition #3164

dheerajbapat opened this issue Jun 12, 2017 · 9 comments

Comments

@dheerajbapat
Copy link

As joinCondition loops over this.body.nodeIndices I am not able to include hidden nodes while forming a cluster.
I think looping over this.body.nodes will solve this.

Scenario: If I have filter on color of nodes (say red nodes are hidden) & I cluster by shape of node.
Then I change filter to show red nodes. Those node will appear out of cluster.
Because those were not included in cluster as it was hidden while forming cluster.

Workaround: process clustering again while changing filters & process filters again while opening cluster.

@wimrijnders
Copy link
Contributor

As joinCondition loops over this.body.nodeIndices I am not able to include hidden nodes while forming a cluster.

This is, in fact, the intended purpose of network.body.nodeIndices. Only the id's of the visible nodes are in this list. The full list of nodes, as you correctly point out, is in this.body.nodes.

I don't fully understand your scenario, but I think the intention is to use clustered nodes to create new clusters, is this correct?

In that case, I feel that your workaround is the correct way to do this. If your think otherwise, please let me know.

@dheerajbapat
Copy link
Author

Thanks for the response.
My scenario is here: http://jsfiddle.net/agfqf6mt/1/
Step 1: Click 'Toggle Red Nodes'
Step 2: Click 'Toggle Cluster Square'
Step 3: Click 'Toggle Red Nodes'

In this case joinCondition does not consider hidden nodes.
How to consider hidden nodes as well while clustering?

My guess was looping over this.body.nodes (in vis cluster function) will solve this.

Or is there any better way to achieve this?

@wimrijnders
Copy link
Contributor

wimrijnders commented Jul 3, 2017

@dheerajbapat Thanks for the great demo!

Update: The problems in the demo stem from the fact that clustering uses field hidden internally as well, and it assumes that it is the sole user of this field. I don't think a complete solution to this issue is possible, but will see if there is a workaround. Will continue to investigate later.

@wimrijnders
Copy link
Contributor

wimrijnders commented Jul 7, 2017

Notes during analysis

There's more going on here than I presumed. I'll keep a list here and expand it as needed.

  • network.GetconnectedEdges() also returns the cluster edges for a given clustered node. Due to the update of the hidden field, the edges DataSet get contaminated by these. Cluster edges must be skipped.
  • clustered nodes are hidden already; their hidden state should not be toggled in toggleRedFilterEle()
  • for a given edge, field hidden shouldn't be toggled if the other node is clustered or hidden.

Order is important: A hidden node does not get clustered. So after clustering, the red square gets reshown on 'Toggle Red Nodes'.

And now I look at the title of this issue and see that this is exactly what it's about. Hm.


This escalated badly; I'm seriously thinking to add hide/unhide functionality to Network. I don't think the casual user wants to be confronted by this.

@dheerajbapat
Copy link
Author

Thanks for understanding and taking it on priority 😄

@wimrijnders
Copy link
Contributor

Interim note: I feel like I'm making a mess of fixing this.

The states hidden and clustered are actually two different things; the problem here is that the hidden state is sort of used to identify both these states in the current code.

I've been trying to consolidate the code to a state in which it works as expected. I've just come to the realization that it would be much better to just accept that hidden and clustered are two different things and that they should be identified and handled separately.

OK, so it's back to the drawing board for me.

@wimrijnders
Copy link
Contributor

wimrijnders commented Jul 20, 2017

OK, I've figured it out. Here's a demo, derived from yours. Please check if it works as expected.

I'll put further information in PR #3274.

Note that in the demo you don't have to explicitly hide the edges any more; this is now done within Network (as it's supposed to!).

wimrijnders added a commit to wimrijnders/vis that referenced this issue Jul 20, 2017
Fix for almende#3164

- `network.clustering.cluster()` now handles all nodes, not just the visible ones
- Changing ivisibility of nodes now explicitly takes clustering into account, see `Network._updateVisibleIndices()`
- `network.clustering` does not change `hidden` status any more.

The important part of this PR is the realization that 'hidden' and 'clustered' are two distinct things and should be handled separately.
In particular, clustering should **not** change the `hidden` state in any way.
yotamberk pushed a commit that referenced this issue Jul 20, 2017
Fix for #3164

- `network.clustering.cluster()` now handles all nodes, not just the visible ones
- Changing ivisibility of nodes now explicitly takes clustering into account, see `Network._updateVisibleIndices()`
- `network.clustering` does not change `hidden` status any more.

The important part of this PR is the realization that 'hidden' and 'clustered' are two distinct things and should be handled separately.
In particular, clustering should **not** change the `hidden` state in any way.
@dheerajbapat
Copy link
Author

Thanks!

I could not check your demo, because it is giving below exception in console.
Refused to execute script from 'https://raw.githubusercontent.com/wimrijnders/github_bugs/master/vis/issue3164/vis.js' because its MIME type ('text/plain') is not executable, and strict MIME type checking is enabled.

Then I saved your vis.js and it is working great!
Thanks for fixing.

@dheerajbapat
Copy link
Author

Still have the problem with latest vis 4.21.0
http://jsfiddle.net/agfqf6mt/3/

primozs pushed a commit to primozs/vis that referenced this issue Jan 3, 2019
Fix for almende#3164

- `network.clustering.cluster()` now handles all nodes, not just the visible ones
- Changing ivisibility of nodes now explicitly takes clustering into account, see `Network._updateVisibleIndices()`
- `network.clustering` does not change `hidden` status any more.

The important part of this PR is the realization that 'hidden' and 'clustered' are two distinct things and should be handled separately.
In particular, clustering should **not** change the `hidden` state in any way.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants