Consolidate network components #1052
Conversation
|
@asizemore I will start to review this in the next day or two. Let me know if you want me to take a look sooner! |
|
No problem, thanks @dmfalke ! This isn't for b68 so can definitely wait |
There was a problem hiding this comment.
I think this looks great. I like the direction you went with this.
Have you given any thought into how various layout algorithms will be applied, in the future? And would a bipartite network be considered a layout? I don't really have any strong opinions on this, but I think it's an interesting thing to consider.
| // In order to assign coordinates to each node, we'll separate the | ||
| // nodes based on their partition, then will use their order in the partition | ||
| // (given by partitionXNodeIDs) to finally assign the coordinates. | ||
| const nodesByPartition: NodeData[][] = useMemo( | ||
| () => | ||
| partition(nodes, (node) => { | ||
| return partitions[0].nodeIds.includes(node.id); | ||
| }), | ||
| [nodes, partitions] | ||
| ); | ||
|
|
||
| const nodesByPartitionWithCoordinates = useMemo( | ||
| () => | ||
| nodesByPartition.map((partition, partitionIndex) => { | ||
| const partitionWithCoordinates = partition.map((node) => { | ||
| // Find the index of the node in the partition | ||
| const indexInPartition = partitions[partitionIndex].nodeIds.findIndex( | ||
| (id) => id === node.id | ||
| ); | ||
|
|
||
| return { | ||
| // partitionIndex of 0 refers to the left-column nodes whereas 1 refers to right-column nodes | ||
| x: partitionIndex === 0 ? column1Position : column2Position, | ||
| y: svgStyles.topPadding + svgStyles.nodeSpacing * indexInPartition, | ||
| labelPosition: | ||
| partitionIndex === 0 ? 'left' : ('right' as LabelPosition), | ||
| ...node, | ||
| }; | ||
| }); | ||
| return partitionWithCoordinates; | ||
| }), | ||
| [ | ||
| column1Position, | ||
| column2Position, | ||
| partitions, | ||
| nodesByPartition, | ||
| svgStyles.nodeSpacing, | ||
| svgStyles.topPadding, | ||
| ] | ||
| ); |
There was a problem hiding this comment.
I think there is potential to make this a little more efficient. In my quick reading, it appears we don't really need to partition the nodes, since we just concatenate the arrays below. What would happen if you got rid of nodesByPartition? You would still need to know which partition the node is in--currently this is done with partitionIndex, which could be replaced with a lookup in partitions.
If we anticipate a large number of nodes (say, in the neighborhood of 1000), this change might be worth considering.
| nodes.map((node, index) => ({ | ||
| labelPosition: 'right' as LabelPosition, | ||
| ...node, | ||
| x: node.x ?? 230 + 200 * Math.cos(2 * Math.PI * (index / nodes.length)), | ||
| y: node.y ?? 230 + 200 * Math.sin(2 * Math.PI * (index / nodes.length)), | ||
| actions: getNodeActions?.(node.id), | ||
| })), |
There was a problem hiding this comment.
It looks like this is conditionally applying a default layout. What do you think about having this component assume that the nodes it receives already have a layout applied, and then we can skip this traversal?
There was a problem hiding this comment.
Skipped in the latest commits
|
@dmfalke ready for another look! I let Your point about the bipartite network being a layout is a good one. Mathematically it's much more than a layout because it carries a bunch of assumptions and rules about the network. But for this codebase, how much does that matter? Is it really anything more than a nice layout of the network when special data is given? I've been asking myself this a lot. But anyways to get back to your question about layouts, yes I have given that a bunch of thought lately. Right now I'd like the backend to do it and to send x/y coordinates back to the frontend. I also want the backend to (someday) include node labels in that layout calculation and send us those coordinates as well so that we don't have a bunch of overlapping labels. The frontend could do a lot of layouts, but I think the backend would be faster and more easily expanded to specialized layouts if we go that direction in the future. |
|
This all sounds great! |
Resolves #996
The goal here is to simplify our network plot components. The approach I've taken here deviates slightly from the original proposal, mostly because cramming all the logic into NetworkPlot became overwhelming. So we'll see what others think!
Left to do:
NetworkPlotpartitionsfromNetworkPlot. That prop is only used to set node positions, and ifBipartiteNetworkPlotdoes that already...Left for later because they're more feature-upgrade types:
NodeWithLabelto take a label position, rotation, etc.Other notes:
svgStyleshandles thecontainerSylesheight and width. It assumes the input is a number, and anything else provided gets the boot (the default boot, here). I don't have a much better way to handle this right now, but just wanted to state my frustration