Add self-correlation app and network visualization #1090
Conversation
|
Hi @asizemore - probably not related to this ticket but I think the SparCC compute might either be very slow or stuck/broken?
|
| return []; | ||
| } | ||
| }, | ||
| // makeGetNodeMenuActions(studyMetadata) { |
There was a problem hiding this comment.
Is there any reason to keep this huge commented region in?
There was a problem hiding this comment.
fixed in a recent commit!
| const NodeData = type({ | ||
| id: string, | ||
| const NetworkConfig = partial({ | ||
| variables: any, |
There was a problem hiding this comment.
unknown would be a safer option. See https://www.typescriptlang.org/docs/handbook/2/functions.html#unknown.
packages/libs/eda/src/lib/core/components/computations/plugins/selfCorrelation.tsx
Outdated
Show resolved
Hide resolved
| const correlationMethodSelectorText = useMemo(() => { | ||
| if (configuration.correlationMethod) { | ||
| return ( | ||
| CORRELATION_METHODS.find( | ||
| (method) => method.value === configuration.correlationMethod | ||
| )?.displayName ?? 'Select a method' | ||
| ); | ||
| } else { | ||
| return 'Select a method'; | ||
| } | ||
| }, [configuration.correlationMethod]); |
There was a problem hiding this comment.
I would lose the useMemo here. The return value of the callback is a string, and the same string values will always be referentially equal. Also, the CORRELATION_METHODS array is small, so traversing it will not have a large impact on performance.
|
I've tested the code, and I think it looks great! Any thoughts on a new icon, since the current one is bipartite?
|
…/selfCorrelation.tsx Co-authored-by: Dave Falke <[email protected]>
…DB/web-monorepo into feature-1038-network-viz
|
Thank you both for taking a look! All changes incorporated in the recent commits. Since they were pretty straightforward, I'll go ahead and merge! |
Yes! It's super slow. Danielle is hoping to find time to see if she can speed it up.
No big thoughts yet, but I'll give a new icon a go later today or next week! But definitely yes, you're right, we do need a new icon! |
Resolves #1038
Progress!
To Do:
To become later issues: