Feature 556 connect correlation backend #560

asizemore · 2023-10-20T17:53:15Z

Resolves #556

This PR adds the correlationAssayMetadata plugin and the BipartiteNetworkVisualization, as well as necessary upgrades to get that all working.

Some things to note:

I had a bit of trouble managing the sizing of the plot. In some ways it should adapt to the container but in others it shouldn't.... check out what i've done and please let me know if it's nonsensical
I intentionally haven't constrained any collections at this point. There's still a discussion about how to handle the different types of data so for now it's just open. This is also why there's no input for method selection at this point either. It all just defaults to "spearman".
Handling link colors took a bit of thinking, so let me know if what i've written isn't clear.
I made up a way to have a "blank" plot to show while there is no data. A blank plot for a network is a little funk since we have no axes, so i just put a few gray nodes around to suggest something might be there soon.

Left for later

truncating long labels. That should be a bpnet component feature, and doesn't deal with the plugin at all i wouldn't think. Use truncateWithEllipsis also add the full label on hover.
handle when an empty network is returned.
Adding an input for changing the correlation coefficient threhsold (but the structure is there)
A real viz picker icon for the bipartite network

Currently working with the qa backend!

Screen.Recording.2023-11-15.at.7.24.52.AM.mov

d-callan · 2023-10-24T17:55:25Z

packages/libs/eda/src/lib/core/api/DataClient/types.ts

-      strokeWidth: number,
-      color: string,
+      linkColor: string,
+      linkWeight: string,


why is this a string?

d-callan · 2023-10-24T17:58:10Z

...eda/src/lib/core/components/visualizations/implementations/BipartiteNetworkVisualization.tsx

+      if (filteredCounts.pending || filteredCounts.value == null)
+        return undefined;
+
+      // There are _no_ viz request params for the volcano plot (config: {}).


this references volcano plot, which this is not. thats not a big deal in itself, but i wonder if this suggests that this operation could be refactored somehow, since this is clearly duplicated code.

d-callan · 2023-10-24T18:00:01Z

...eda/src/lib/core/components/visualizations/implementations/BipartiteNetworkVisualization.tsx

+        return undefined;
+
+      // There are _no_ viz request params for the volcano plot (config: {}).
+      // The data service streams the volcano data directly from the compute service.


another reference to volcano here. this time though i think my larger point is that for the network i think we said itd end up being the data service job to do the pruning of edges by pvalue and correlation coefficient thresholds. (i dont actually remember the compute or data service prs well enough to remember if thats what we actually ended up doing. but think we should. it was a good plan.)

anyway, just trying to say there will be viz plugin params, eventually, if not yet.

dmfalke · 2023-11-15T22:41:14Z

I just viewed the screen cast. Here are some initial thoughts (I will review the code next):

The empty plot looks good in the viz, but I'm concerned about it being used as a thumbnail in the listing page. I don't have any suggestions for that, at the moment.
The scrolling is okay, but it would be really cool if you could scroll each column, independently. I have no idea if that's possible.

dmfalke

I've reviewed the code, and it looks great. I have one small suggestion, which you can find below.

When I was testing the new viz, I noticed something odd. A thumbnail is being generated constantly, even without interacting with the plot.

It's not clear what is causing this. Let me know if you want help figuring it out.

I'm also finding the text in the plot a bit hard to read. Maybe it's my browser (Firefox)?

Finally, I didn't see any tooltips. Is that intentional?

I tested using the DailyBaby study.

Great work!

...eda/src/lib/core/components/visualizations/implementations/BipartiteNetworkVisualization.tsx

…mentations/BipartiteNetworkVisualization.tsx Co-authored-by: Dave Falke <[email protected]>

jernestmyers · 2023-11-16T16:59:40Z

...eda/src/lib/core/components/visualizations/implementations/BipartiteNetworkVisualization.tsx

+        };
+      }),
+    };
+  }, [data, entities, linkColorScale]);


I think linkColorScale is causing the infinite loop with the thumbnail and patch request. All of the logic for linkColorScale is outside of the useMemo and new renders aren't guaranteed to be referentially equal. As I see it, we can either:

move the logic for uniqueLinkColors, linkColorScaleDomain and linkColorScale inside of this useMemo and update the dependency array accordingly (I think this may be as simple as removing linkColorScale from the deps)

keep the logic where it is but remove linkColorScale from the dependency array; this will upset ESLint, plus we'd be re-running the logic for those variables more than we need to

I vote for option 1 and I also suggest we specify data.value instead of just data in this dependency array while we're at it.

(Edited for a bit more clarity!)

asizemore · 2023-11-16T19:52:22Z

Thank you both!! I'm sorry i wasn't able to get to this today after you both were so fast with your feedback, but i'll focus on the updates first thing tomorrow morning :)

asizemore · 2023-11-17T11:33:26Z

@jernestmyers you were right! The colorscale was the issue! Thanks for the find!

@dmfalke responses to your questions below!

The empty plot looks good in the viz, but I'm concerned about it being used as a thumbnail in the listing page. I don't have any suggestions for that, at the moment.

This is a great point, i hadn't thought about that. A user will see this only if the computation has finished but they haven't opened the viz yet (the same scenario for diff abund is shown above just for comparison). The little gray network isn't totally ridiculous looking, which was my big fear when you mentioned it, but it also doesn't really communicate "hey click on me i'm an empty plot". I also don't have a better way to solve that yet, other than just "Empty network" text? It might need some mockups and discussion.

Finally, I didn't see any tooltips. Is that intentional?

Yes, it was intentional. As i was going i was looking for ways to keep this PR reasonably sized and since the scope of tooltips is just the component i felt like it could get cut from this pr that focuses on the viz + app plugin. Happy to add them if that would be preferable!

The scrolling is okay, but it would be really cool if you could scroll each column, independently. I have no idea if that's possible.

That would be super cool!!! I don't know either but something I'm on the lookout for. During the design phase, we had assumed that independent column scrolling would be hard, so we're hoping that implementing user-filters on nodes and edges will be sufficient. But we'll see... 😄

I'm also finding the text in the plot a bit hard to read. Maybe it's my browser (Firefox)?

The glyphs were pretty thin so I increased the font weight of all the text (see below). Is that a little easier to read? I could also increase the font size, too! Let me know what you think.

jernestmyers

These changes LGTM. Not seeing anymore wayward requests and the text is certainly easier to see!

dmfalke · 2023-11-20T15:57:37Z

These updates look great. Very nice!

asizemore added 3 commits October 17, 2023 06:02

successful correlation compute

b7fd6ef

working bipartiteNetwork response

3319fb9

add node display labels

e87fc54

d-callan reviewed Oct 24, 2023

View reviewed changes

asizemore added 12 commits November 10, 2023 06:31

Merge branch 'main' into feature-556-connect-correlation-backend

6ebb625

update api to match new ds api

d3b95df

update to new api

d5c0480

one more api update

f1975fa

add plot subtitle to bp net

28798c9

Merge branch 'main' into feature-556-connect-correlation-backend

d6e3add

add loading spinner

e634dcf

add styles to viz

b6eea05

add empty-ish bpnet data

4d9564f

Add svg styling and handle overrides

a353a29

clean and document

a060b3f

Merge branch 'main' into feature-556-connect-correlation-backend

0c609f2

asizemore marked this pull request as ready for review November 15, 2023 11:51

asizemore requested review from dmfalke and jernestmyers November 15, 2023 11:51

dmfalke reviewed Nov 16, 2023

View reviewed changes

...eda/src/lib/core/components/visualizations/implementations/BipartiteNetworkVisualization.tsx Outdated Show resolved Hide resolved

Update packages/libs/eda/src/lib/core/components/visualizations/imple…

d494684

…mentations/BipartiteNetworkVisualization.tsx Co-authored-by: Dave Falke <[email protected]>

jernestmyers reviewed Nov 16, 2023

View reviewed changes

asizemore added 3 commits November 17, 2023 06:23

fix constant thumbnail generation

bad85a3

increase weight of bpnet text

f8944a9

Merge branch 'main' into feature-556-connect-correlation-backend

cf70668

asizemore requested review from dmfalke and jernestmyers November 20, 2023 11:00

jernestmyers approved these changes Nov 20, 2023

View reviewed changes

Merge branch 'main' into feature-556-connect-correlation-backend

142587c

asizemore merged commit 1257cb3 into main Nov 20, 2023
1 check passed

asizemore deleted the feature-556-connect-correlation-backend branch November 20, 2023 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature 556 connect correlation backend #560

Feature 556 connect correlation backend #560

asizemore commented Oct 20, 2023 •

edited

Loading

d-callan Oct 24, 2023

d-callan Oct 24, 2023

d-callan Oct 24, 2023

d-callan Oct 24, 2023

dmfalke commented Nov 15, 2023

dmfalke left a comment

jernestmyers Nov 16, 2023 •

edited

Loading

asizemore commented Nov 16, 2023

asizemore commented Nov 17, 2023

jernestmyers left a comment

dmfalke commented Nov 20, 2023

Feature 556 connect correlation backend #560

Feature 556 connect correlation backend #560

Conversation

asizemore commented Oct 20, 2023 • edited Loading

d-callan Oct 24, 2023

Choose a reason for hiding this comment

d-callan Oct 24, 2023

Choose a reason for hiding this comment

d-callan Oct 24, 2023

Choose a reason for hiding this comment

d-callan Oct 24, 2023

Choose a reason for hiding this comment

dmfalke commented Nov 15, 2023

dmfalke left a comment

Choose a reason for hiding this comment

jernestmyers Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

asizemore commented Nov 16, 2023

asizemore commented Nov 17, 2023

jernestmyers left a comment

Choose a reason for hiding this comment

dmfalke commented Nov 20, 2023

asizemore commented Oct 20, 2023 •

edited

Loading

jernestmyers Nov 16, 2023 •

edited

Loading