-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Disable apps based on study metadata #732
Changes from all commits
7753a04
d11d444
95df6b7
0ede162
c0e7810
c085f10
6dd33b9
0b5c548
886a994
00dce43
cf43be4
f65b41a
a5ce02e
61b61b7
d01252d
e87e21e
7514540
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,8 @@ import { bipartiteNetworkVisualization } from '../../visualizations/implementati | |
import { variableCollectionsAreUnique } from '../../../utils/visualization'; | ||
import PluginError from '../../visualizations/PluginError'; | ||
import { VariableCollectionSelectList } from '../../variableSelectors/VariableCollectionSingleSelect'; | ||
import { IsEnabledInPickerParams } from '../../visualizations/VisualizationTypes'; | ||
import { entityTreeToArray } from '../../../utils/study-metadata'; | ||
|
||
const cx = makeClassNameHelper('AppStepConfigurationContainer'); | ||
|
||
|
@@ -73,6 +75,9 @@ export const plugin: ComputationPlugin = { | |
}, | ||
}), // Must match name in data service and in visualization.tsx | ||
}, | ||
isEnabledInPicker: isEnabledInPicker, | ||
studyRequirements: | ||
'These visualizations are only available for studies with metagenomic data.', | ||
}; | ||
|
||
// Renders on the thumbnail page to give a summary of the app instance | ||
|
@@ -198,3 +203,21 @@ export function CorrelationAssayAssayConfiguration( | |
</ComputationStepContainer> | ||
); | ||
} | ||
|
||
// The correlation assay x assay app should only be available | ||
// for studies with metagenomic data. | ||
function isEnabledInPicker({ | ||
studyMetadata, | ||
}: IsEnabledInPickerParams): boolean { | ||
if (!studyMetadata) return false; | ||
|
||
const entities = entityTreeToArray(studyMetadata.rootEntity); | ||
|
||
// Check that the metagenomic entity exists _and_ that it has | ||
// at least one collection. | ||
const hasMetagenomicData = entities.some( | ||
(entity) => entity.id === 'OBI_0002623' && !!entity.collections?.length | ||
); // OBI_0002623 = Metagenomic sequencing assay | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also need to check for non-zero length There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or I guess There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack you're right! All fixed now! I was interested so I looked into the difference in performance between |
||
return hasMetagenomicData; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,9 @@ import { makeClassNameHelper } from '@veupathdb/wdk-client/lib/Utils/ComponentUt | |
import { H6 } from '@veupathdb/coreui'; | ||
import { bipartiteNetworkVisualization } from '../../visualizations/implementations/BipartiteNetworkVisualization'; | ||
import { VariableCollectionSelectList } from '../../variableSelectors/VariableCollectionSingleSelect'; | ||
import { entityTreeToArray } from '../../../utils/study-metadata'; | ||
import { IsEnabledInPickerParams } from '../../visualizations/VisualizationTypes'; | ||
import { ancestorEntitiesForEntityId } from '../../../utils/data-element-constraints'; | ||
|
||
const cx = makeClassNameHelper('AppStepConfigurationContainer'); | ||
|
||
|
@@ -62,6 +65,9 @@ export const plugin: ComputationPlugin = { | |
}, | ||
}), // Must match name in data service and in visualization.tsx | ||
}, | ||
isEnabledInPicker: isEnabledInPicker, | ||
studyRequirements: | ||
'These visualizations are only available for studies with compatible metadata.', | ||
}; | ||
|
||
// Renders on the thumbnail page to give a summary of the app instance | ||
|
@@ -155,3 +161,65 @@ export function CorrelationAssayMetadataConfiguration( | |
</ComputationStepContainer> | ||
); | ||
} | ||
|
||
const ASSAY_ENTITIES = [ | ||
'OBI_0002623', | ||
'EUPATH_0000809', | ||
'EUPATH_0000813', | ||
'EUPATH_0000812', | ||
]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Also open to other solutions! |
||
|
||
// The correlation assay x metadata app is only available for studies | ||
// with appropriate metadata. Specifically, the study | ||
// must have at least one continuous metadata variable that is on a one-to-one path | ||
// from the assay entity. | ||
// See PR #74 in service-eda-compute for the matching logic on the backend. | ||
function isEnabledInPicker({ | ||
studyMetadata, | ||
}: IsEnabledInPickerParams): boolean { | ||
if (!studyMetadata) return false; | ||
|
||
const entities = entityTreeToArray(studyMetadata.rootEntity); | ||
// Ensure there are collections in this study. Otherwise, disable app | ||
const studyHasCollections = entities.some( | ||
(entity) => !!entity.collections?.length | ||
); | ||
if (!studyHasCollections) return false; | ||
|
||
// Check for appropriate metadata | ||
// Step 1. Find the first assay node. Doesn't need to be any assay in particular just any mbio assay will do | ||
const firstAssayEntityIndex = entities.findIndex((entity) => | ||
ASSAY_ENTITIES.includes(entity.id) | ||
); | ||
if (firstAssayEntityIndex === -1) return false; | ||
|
||
// Step 2. Find all ancestor entites of the assayEntity that are on a one-to-one path with assayEntity. | ||
// Step 2a. Grab ancestor entities. | ||
const ancestorEntities = ancestorEntitiesForEntityId( | ||
entities[firstAssayEntityIndex].id, | ||
entities | ||
).reverse(); // Reverse so that the ancestorEntities[0] is the assay and higher indices are further up the tree. | ||
|
||
// Step 2b. Trim the ancestorEntities so that we only keep those that are on | ||
// a 1:1 path. Once we find an ancestor that is many to one with its parent, we | ||
// know we've hit the end of the 1:1 path. | ||
const lastOneToOneAncestorIndex = ancestorEntities.findIndex( | ||
(entity) => entity.isManyToOneWithParent | ||
); | ||
const oneToOneAncestors = ancestorEntities.slice( | ||
1, // removing the assay itself | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we assume there are no continuous 'metadata variables' on the same entity as the assay entity? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right. That assay entity should only have assay data. |
||
lastOneToOneAncestorIndex + 1 | ||
); | ||
|
||
// Step 3. Check if there are any continuous variables in the filtered entities | ||
const hasContinuousVariable = oneToOneAncestors | ||
.flatMap((entity) => entity.variables) | ||
.some( | ||
(variable) => | ||
'dataShape' in variable && | ||
variable.dataShape === 'continuous' && | ||
(variable.type === 'number' || variable.type === 'integer') // Support for dates coming soon! Can remove this line once the backend is ready. | ||
); | ||
|
||
return hasContinuousVariable; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. Simplified in the latest commit!