Disable apps based on study metadata #732

asizemore · 2023-12-14T19:08:55Z

Resolves #731 , child of #407

The two correlation apps require particular data to run. Some studies don't have these data, so for those cases we want to show a disabled app. More specifically,

correlation assay x metadata requires at least one continuous metadata variable on an entity that is on a one-to-one path with the assay
correlation assay x assay is specifically for taxa vs functional data. "Functional data" means data from a metagenomic assay, which is an entity that not all studies have.

I also used the same architecture for the above checks to address #723

To Dos

add some isEnabled function to the apps
disable all vizs in the app if the app is disabled
can also do this one Hide apps if no collections exist #723 ?
have the app give a reason that it's not available
remove all the ts-ignores :)
repeat for correlation assay v metadata
improve comments

For testing, some helpful studies include:

BONUS. Both corrleation apps should be available
DailyBaby. Should have only the assay x metadata correlation app
HMP WGS. Should have only the assay x assay (Functional associations) app
HMP V1-V3. Should have neither app

asizemore · 2023-12-19T11:39:47Z

Dear reviewers,

Please pay close attention to

The logic in correlationAssayMetadata.tsx for checking the right variables
The typing of entityTreeToArray output to ensure that wouldn't break anything else

Thank you!

asizemore · 2023-12-19T15:16:47Z

@reviewers i added some studies in the description to help with testing

dmfalke

The code looks great. Just a couple of suggestions.

I'm not crazy about the look:

It sort of looks like that app is selected.

What about something like this:

I removed the background color, and dropped the opacity to 0.4.

packages/libs/eda/src/lib/core/components/computations/plugins/abundance.tsx

packages/libs/eda/src/lib/core/components/computations/plugins/correlationAssayMetadata.tsx

Co-authored-by: Dave Falke <[email protected]>

asizemore · 2023-12-19T18:57:57Z

I removed the background color, and dropped the opacity to 0.4.

Ahh this looks much much better. Good idea. I'll see if i can get the "Not applicable..." text to show up a bit more... to be continued!

And @dmfalke thank you for your other suggestions - they've all been added!

bobular

The code looks great but no correlation apps are enabled for DailyBaby 😞

It should have the Correlation "Discover taxa or genes correlated with metadata" app, right? Because it has 1:1:1 PRM:Samples:Assays and there is a continuous Age variable on PRM.

bobular · 2023-12-19T21:41:04Z

packages/libs/eda/src/lib/core/components/computations/plugins/abundance.tsx

+  const studyHasCollections = entities.some(
+    (e): e is StudyEntity & Required<Pick<StudyEntity, 'collections'>> =>
+      !!e.collections?.length
+  );


How important is the type predicate assertion here? If we were filtering and then doing something with the result (especially if an array result of StudyEntities), it would be useful. But I'm not sure the extra complication wins us anything here because the some just returns true/false.

True. Simplified in the latest commit!

bobular · 2023-12-19T21:43:14Z

packages/libs/eda/src/lib/core/components/computations/plugins/correlationAssayAssay.tsx

+  const entities = entityTreeToArray(studyMetadata.rootEntity);
+  const hasMetagenomicData =
+    entities.filter((entity) => entity.id === 'OBI_0002623').length > 0; // OBI_0002623 = Metagenomic sequencing assay
+


Do we also need to check for non-zero length collections here?

you're right we should. I was making the assumption that if that entity existed, then it would have a collection. But just becasue that's generally true doesn't mean it will always be the case! I'll add a check

Also, an entities.find(...) !== undefined might be very very slightly marginally more efficient! (because it stops the search as soon as it finds something - but these arrays are tiny, so it will make very little difference)

Or I guess entities.some(...) works too!

It's still possible that the study with collections and a OBI_0002623 assay with no collections could get through, I think?

Maybe just this?

const hasMetagenomicData = entities.some((entity) => entity.id === 'OBI_0002623' && !!entity.collections?.length);

ack you're right! All fixed now!

I was interested so I looked into the difference in performance between some and find. It seems some is ever so slightly more efficient, since find calls some. But you're right it wouldn't matter on an array of this size anyway.

bobular · 2023-12-19T21:49:30Z

packages/libs/eda/src/lib/core/components/computations/plugins/correlationAssayMetadata.tsx

+  'EUPATH_0000809',
+  'EUPATH_0000813',
+  'EUPATH_0000812',
+];


Is there a long term plan (e.g. ticket) to annotate entities appropriately to reduce the amount of hardcoded entity IDs? (If that's agreed to be a plan.)

Great question. I just made a ticket for it but i think it could use some more discussion. There wasn't originally a long-term plan because i didn't think that i would end up needing these assay ids at all until i got into the implementation.

To completely remove the hardcoded ids we'd need to add the following annotations:

To the study, add hasMetagenomicData. I think there's a similar hasMapData annotation?

To each entity, add isAssay. Or i suppose it could be something like entityType = 'assay', if there are other entity types that might be useful to us.

Also open to other solutions!

bobular · 2023-12-19T22:00:37Z

packages/libs/eda/src/lib/core/components/computations/plugins/correlationAssayMetadata.tsx

+    (entity) => entity.isManyToOneWithParent
+  );
+  const oneToOneAncestors = ancestorEntities.slice(
+    1, // removing the assay itself


So we assume there are no continuous 'metadata variables' on the same entity as the assay entity?

Right. That assay entity should only have assay data.

asizemore · 2023-12-20T11:24:49Z

@dmfalke what do you think of the below? I faked transparency by lightening the gray of the text to a hue that matches what it'd look like if it was transparent.
The implementation isn't elegant, though. Check it out in the latest commit!

asizemore · 2023-12-20T11:41:29Z

no correlation apps are enabled for DailyBaby 😞

Oh no! Turns out that Age variable is an integer, not a number, so i was missing it. Fixed in the latest commit. Good catch @bobular !

bobular · 2023-12-20T12:10:42Z

Transparency faking looks good to me. I don't think there's another way.

bobular

Looks good to me (though I haven't fired it up again to test).

I left one pedantic logic query.

bobular · 2023-12-20T12:28:59Z

packages/libs/eda/src/lib/core/components/computations/plugins/correlationAssayAssay.tsx

+  const entities = entityTreeToArray(studyMetadata.rootEntity);
+  const hasMetagenomicData =
+    entities.filter((entity) => entity.id === 'OBI_0002623').length > 0; // OBI_0002623 = Metagenomic sequencing assay
+


It's still possible that the study with collections and a OBI_0002623 assay with no collections could get through, I think?

Maybe just this?

const hasMetagenomicData = entities.some((entity) => entity.id === 'OBI_0002623' && !!entity.collections?.length);

asizemore added 6 commits December 14, 2023 13:57

figure out mechanics of disabled app style

7753a04

disable all vizs in an app if app is disabled

d11d444

Let app plugin tell StartPage why it is disabled

95df6b7

draft corr assay metadata requirements logic

0ede162

Merge branch 'main' into feature-731-disable-apps-by-study

c0e7810

type output of entityTreeToArray and remove tsignores

c085f10

asizemore mentioned this pull request Dec 18, 2023

Parent issue: polishing updates for bipartite network #667

Closed

10 tasks

asizemore added 3 commits December 18, 2023 14:00

remove ts-ignores and unused imports

6dd33b9

disable compute apps if no collection exists

0b5c548

cleanup and improve comments

886a994

asizemore marked this pull request as ready for review December 19, 2023 11:37

Merge branch 'main' into feature-731-disable-apps-by-study

00dce43

asizemore requested review from dmfalke and bobular December 19, 2023 11:37

dmfalke reviewed Dec 19, 2023

View reviewed changes

packages/libs/eda/src/lib/core/components/computations/plugins/abundance.tsx Outdated Show resolved Hide resolved

packages/libs/eda/src/lib/core/components/computations/plugins/correlationAssayMetadata.tsx Outdated Show resolved Hide resolved

Apply suggestions from code review

cf43be4

Co-authored-by: Dave Falke <[email protected]>

improve logic finding if a study has collections

f65b41a

bobular reviewed Dec 19, 2023

View reviewed changes

test disable styling

a5ce02e

asizemore mentioned this pull request Dec 20, 2023

Add annotations to mbio data to help determine appropriate apps VEuPathDB/lib-eda-subsetting#43

Open

allow continuous integers in metadata corr app

61b61b7

simplify logic for finding if a study has collections

d01252d

asizemore requested review from bobular and dmfalke December 20, 2023 12:15

bobular approved these changes Dec 20, 2023

View reviewed changes

asizemore added 2 commits December 20, 2023 10:03

fix logic loophole checking metagenomic entity for collections

e87e21e

Merge branch 'main' into feature-731-disable-apps-by-study

7514540

asizemore merged commit 1f0a13d into main Dec 20, 2023
1 check passed

asizemore deleted the feature-731-disable-apps-by-study branch December 20, 2023 15:28

asizemore mentioned this pull request Jan 10, 2024

Hide apps if no collections exist #723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable apps based on study metadata #732

Disable apps based on study metadata #732

asizemore commented Dec 14, 2023 •

edited

Loading

asizemore commented Dec 19, 2023

asizemore commented Dec 19, 2023

dmfalke left a comment

asizemore commented Dec 19, 2023

bobular left a comment

bobular Dec 19, 2023

asizemore Dec 20, 2023

bobular Dec 19, 2023

asizemore Dec 20, 2023

bobular Dec 20, 2023

bobular Dec 20, 2023

bobular Dec 20, 2023

asizemore Dec 20, 2023

bobular Dec 19, 2023

asizemore Dec 20, 2023

bobular Dec 19, 2023

asizemore Dec 20, 2023

asizemore commented Dec 20, 2023

asizemore commented Dec 20, 2023

bobular commented Dec 20, 2023 •

edited

Loading

bobular left a comment

bobular Dec 20, 2023

Disable apps based on study metadata #732

Disable apps based on study metadata #732

Conversation

asizemore commented Dec 14, 2023 • edited Loading

asizemore commented Dec 19, 2023

asizemore commented Dec 19, 2023

dmfalke left a comment

Choose a reason for hiding this comment

asizemore commented Dec 19, 2023

bobular left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asizemore commented Dec 20, 2023

asizemore commented Dec 20, 2023

bobular commented Dec 20, 2023 • edited Loading

bobular left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asizemore commented Dec 14, 2023 •

edited

Loading

bobular commented Dec 20, 2023 •

edited

Loading