From 46501d63d987865d9cb31685080bc76b36c1c336 Mon Sep 17 00:00:00 2001 From: atarashansky Date: Fri, 12 Jul 2024 11:12:09 -0700 Subject: [PATCH] chore(de): final polish for launch (#7278) --- backend/common/census_cube/data/query.py | 14 ++-- backend/de/api/v1.py | 17 ++++- .../components/common/HelpTooltip/index.tsx | 8 ++- .../common/store/reducer.ts | 45 +++++++++++-- .../components/QueryGroupTags/index.tsx | 47 +++++++------- .../DifferentialExpressionResults/connect.ts | 8 +-- .../DifferentialExpressionResults/index.tsx | 22 +++++-- .../DifferentialExpressionResults/style.ts | 12 ++-- .../Main/components/DeResults/style.ts | 6 +- .../components/FilterDropdown/connect.ts | 27 ++++---- .../Main/components/Method/index.tsx | 43 +++++++++++- .../Main/components/Method/style.ts | 11 ++++ .../Main/components/OverlapBehavior/index.tsx | 20 +++--- .../components/Main/index.tsx | 65 +++++++------------ .../components/Main/style.ts | 8 ++- .../differentialExpression.test.ts | 7 +- 16 files changed, 232 insertions(+), 128 deletions(-) diff --git a/backend/common/census_cube/data/query.py b/backend/common/census_cube/data/query.py index 24bd5ae754ba1..afa30a982dcf3 100644 --- a/backend/common/census_cube/data/query.py +++ b/backend/common/census_cube/data/query.py @@ -97,11 +97,9 @@ def cell_counts_diffexp_df(self, criteria: BaseQueryCriteria) -> DataFrame: @tracer.wrap( name="expression_summary_and_cell_counts_diffexp", service="de-api", resource="_query", span_type="de-api" ) - def expression_summary_and_cell_counts_diffexp(self, criteria: BaseQueryCriteria) -> tuple[DataFrame, DataFrame]: - use_simple = not any( - depluralize(key) not in cell_counts_indexed_dims and values for key, values in dict(criteria).items() - ) - + def expression_summary_and_cell_counts_diffexp( + self, criteria: BaseQueryCriteria, use_simple: bool + ) -> tuple[DataFrame, DataFrame]: cell_counts_diffexp_df = self.cell_counts_diffexp_df(criteria) cell_counts_group_id_key = "group_id_simple" if use_simple else "group_id" cube = ( @@ -209,6 +207,12 @@ def list_grouped_primary_filter_dimensions_term_ids( ) +def should_use_simple_group_ids(criteria: BaseQueryCriteria): + return not any( + depluralize(key) not in cell_counts_indexed_dims and values for key, values in dict(criteria).items() + ) + + def depluralize(attr_name): return attr_name[:-1] if attr_name[-1] == "s" else attr_name diff --git a/backend/de/api/v1.py b/backend/de/api/v1.py index eab8fc70d586f..47edd9deda5e7 100644 --- a/backend/de/api/v1.py +++ b/backend/de/api/v1.py @@ -10,7 +10,7 @@ from backend.common.census_cube.data.criteria import BaseQueryCriteria from backend.common.census_cube.data.ontology_labels import gene_term_label, ontology_term_label -from backend.common.census_cube.data.query import CensusCubeQuery +from backend.common.census_cube.data.query import CensusCubeQuery, should_use_simple_group_ids from backend.common.census_cube.data.schemas.cube_schema_diffexp import cell_counts_logical_dims_exclude_dataset_id from backend.common.census_cube.data.snapshot import CensusCubeSnapshot, load_snapshot from backend.common.census_cube.utils import ancestors, descendants @@ -248,8 +248,19 @@ def run_differential_expression( set(sum([descendants(i) for i in criteria2.cell_type_ontology_term_ids], [])) ) - es1, cell_counts1 = q.expression_summary_and_cell_counts_diffexp(criteria1) - es2, cell_counts2 = q.expression_summary_and_cell_counts_diffexp(criteria2) + if exclude_overlapping_cells == "retainBoth": + # If we are not excluding overlapping cells (retainBoth), we can use the simple group IDs where applicable. + es1, cell_counts1 = q.expression_summary_and_cell_counts_diffexp( + criteria1, should_use_simple_group_ids(criteria1) + ) + es2, cell_counts2 = q.expression_summary_and_cell_counts_diffexp( + criteria2, should_use_simple_group_ids(criteria2) + ) + else: + # If we are excluding overlapping cells, we can only use the simple group IDs if both groups are eligible. + use_simple_group_ids = should_use_simple_group_ids(criteria1) and should_use_simple_group_ids(criteria2) + es1, cell_counts1 = q.expression_summary_and_cell_counts_diffexp(criteria1, use_simple_group_ids) + es2, cell_counts2 = q.expression_summary_and_cell_counts_diffexp(criteria2, use_simple_group_ids) n_cells1 = cell_counts1["n_total_cells"].sum() n_cells2 = cell_counts2["n_total_cells"].sum() diff --git a/frontend/src/views/CellGuide/components/CellGuideCard/components/common/HelpTooltip/index.tsx b/frontend/src/views/CellGuide/components/CellGuideCard/components/common/HelpTooltip/index.tsx index 0f253fa908e74..b05b81834812d 100644 --- a/frontend/src/views/CellGuide/components/CellGuideCard/components/common/HelpTooltip/index.tsx +++ b/frontend/src/views/CellGuide/components/CellGuideCard/components/common/HelpTooltip/index.tsx @@ -35,9 +35,10 @@ interface Props { element: JSX.Element; } | null> >; - title: string; + title?: string; skinnyMode?: boolean; extraContent?: JSX.Element; + width?: "wide" | "default"; } const HelpTooltip = ({ text, @@ -48,12 +49,13 @@ const HelpTooltip = ({ title, skinnyMode = false, extraContent, + width = "wide", }: Props) => { return ( {text}} slotProps={getSlotProps(dark)} @@ -67,7 +69,7 @@ const HelpTooltip = ({ { - if (skinnyMode && setTooltipContent) { + if (skinnyMode && setTooltipContent && title) { setTooltipContent({ title: title, element: {text}, diff --git a/frontend/src/views/DifferentialExpression/common/store/reducer.ts b/frontend/src/views/DifferentialExpression/common/store/reducer.ts index 23966177eb19f..e1929885e99f1 100644 --- a/frontend/src/views/DifferentialExpression/common/store/reducer.ts +++ b/frontend/src/views/DifferentialExpression/common/store/reducer.ts @@ -56,7 +56,7 @@ export const EMPTY_FILTERS = { export const INITIAL_STATE: State = { organismId: null, snapshotId: null, - excludeOverlappingCells: "excludeTwo", + excludeOverlappingCells: "retainBoth", queryGroups: { queryGroup1: EMPTY_FILTERS, queryGroup2: EMPTY_FILTERS }, queryGroupsWithNames: { queryGroup1: EMPTY_FILTERS, @@ -204,12 +204,49 @@ function selectQueryGroup2Filters( } function submitQueryGroups(state: State, _: PayloadAction): State { - const { queryGroups, queryGroupsWithNames } = state; + const { selectedOptionsGroup1, selectedOptionsGroup2 } = state; + + const filterAndMapOptions = ( + selectedOptions: Record, + mapTo: "id" | "name" + ) => { + const result: Record = {}; + for (const key in selectedOptions) { + result[key as keyof QueryGroup] = selectedOptions[key as keyof QueryGroup] + .filter((option: FilterOption) => !option.unavailable) + .map((option: FilterOption) => option[mapTo]); + } + return result; + }; + + const newQueryGroup1 = { + ...EMPTY_FILTERS, + ...filterAndMapOptions(selectedOptionsGroup1, "id"), + }; + const newQueryGroup2 = { + ...EMPTY_FILTERS, + ...filterAndMapOptions(selectedOptionsGroup2, "id"), + }; + + const newQueryGroupWithNames1 = { + ...EMPTY_FILTERS, + ...filterAndMapOptions(selectedOptionsGroup1, "name"), + }; + const newQueryGroupWithNames2 = { + ...EMPTY_FILTERS, + ...filterAndMapOptions(selectedOptionsGroup2, "name"), + }; return { ...state, - submittedQueryGroups: queryGroups, - submittedQueryGroupsWithNames: queryGroupsWithNames, + submittedQueryGroups: { + queryGroup1: newQueryGroup1, + queryGroup2: newQueryGroup2, + }, + submittedQueryGroupsWithNames: { + queryGroup1: newQueryGroupWithNames1, + queryGroup2: newQueryGroupWithNames2, + }, }; } diff --git a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/components/QueryGroupTags/index.tsx b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/components/QueryGroupTags/index.tsx index 849311ba6e0e4..b29bee2c0bdeb 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/components/QueryGroupTags/index.tsx +++ b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/components/QueryGroupTags/index.tsx @@ -1,8 +1,5 @@ import { useMemo } from "react"; -import { - QueryGroup, - State, -} from "src/views/DifferentialExpression/common/store/reducer"; +import { QueryGroup } from "src/views/DifferentialExpression/common/store/reducer"; import { Tooltip } from "@czi-sds/components"; import { StyledTag } from "./style"; import { QUERY_GROUP_KEY_TO_TAG_SUFFIX_MAP } from "./constants"; @@ -11,57 +8,59 @@ import { NO_ORGAN_ID } from "src/views/CellGuide/components/CellGuideCard/compon import Link from "src/views/CellGuide/components/CellGuideCard/components/common/Link"; const QueryGroupTags = ({ - selectedOptions, + submittedQueryGroup, + submittedQueryGroupWithNames, }: { - selectedOptions: State["selectedOptionsGroup1"]; + submittedQueryGroup: QueryGroup; + submittedQueryGroupWithNames: QueryGroup; }) => { const nonEmptyQueryGroupKeys = useMemo(() => { - return Object.keys(selectedOptions).filter( - (key) => selectedOptions[key as keyof QueryGroup].length > 0 + return Object.keys(submittedQueryGroup).filter( + (key) => submittedQueryGroup[key as keyof QueryGroup].length > 0 ); - }, [selectedOptions]); + }, [submittedQueryGroup]); return ( <> {nonEmptyQueryGroupKeys.map((key) => { const queryGroupKey = key as keyof QueryGroup; - const selected = selectedOptions[queryGroupKey].filter( - (option) => !option.unavailable - ); + const selectedIds = submittedQueryGroup[queryGroupKey]; + const selectedNames = submittedQueryGroupWithNames[queryGroupKey]; const suffix = QUERY_GROUP_KEY_TO_TAG_SUFFIX_MAP[queryGroupKey]; const label = - selected.length > 1 - ? `${selected.length} ${suffix}` - : selected[0].name; + selectedNames.length > 1 + ? `${selectedNames.length} ${suffix}` + : selectedNames[0]; const getValue = (index: number) => { return key === "cellTypes" ? ( ) : ( - selected[index].name + selectedNames[index] ); }; const clickToViewText = "Click to view in CellGuide"; const tooltipContent = - selected.length === 1 && key === "cellTypes" ? ( + selectedNames.length === 1 && key === "cellTypes" ? ( clickToViewText ) : (
{key === "cellTypes" && {clickToViewText}} - {selected.map((value, index) => ( + {selectedNames.map((value, index) => (
{getValue(index)}
))}
); - const isSingleCellType = key === "cellTypes" && selected.length === 1; + const isSingleCellType = + key === "cellTypes" && selectedNames.length === 1; return ( { const { excludeOverlappingCells, - selectedOptionsGroup1, - selectedOptionsGroup2, + submittedQueryGroups, + submittedQueryGroupsWithNames, } = useContext(StateContext); const [page, setPage] = useState(1); @@ -123,7 +123,7 @@ export const useConnect = ({ datasets2.length !== 1 ? "s" : "" }`, showOverlappingCellsCallout: excludeOverlappingCells === "retainBoth", - selectedOptionsGroup1, - selectedOptionsGroup2, + submittedQueryGroups, + submittedQueryGroupsWithNames, }; }; diff --git a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/index.tsx b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/index.tsx index cfb16d093f809..e3e2c24296eda 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/index.tsx +++ b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/index.tsx @@ -68,8 +68,8 @@ const DifferentialExpressionResults = ({ numDatasetsText1, numDatasetsText2, showOverlappingCellsCallout, - selectedOptionsGroup1, - selectedOptionsGroup2, + submittedQueryGroups, + submittedQueryGroupsWithNames, } = useConnect({ queryGroups, queryGroupsWithNames, @@ -177,6 +177,10 @@ const DifferentialExpressionResults = ({ setPage, ]); + if (!submittedQueryGroups || !submittedQueryGroupsWithNames) { + return null; + } + return ( <> @@ -216,7 +220,12 @@ const DifferentialExpressionResults = ({ - + @@ -256,7 +265,12 @@ const DifferentialExpressionResults = ({ - + {!!errorMessage && ( diff --git a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/style.ts b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/style.ts index 874e1d71d16eb..7e7cd428cff51 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/style.ts +++ b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/components/DifferentialExpressionResults/style.ts @@ -5,12 +5,14 @@ import { TextField } from "@mui/material"; import { inputBaseClasses } from "@mui/material/InputBase"; import { inputLabelClasses } from "@mui/material/InputLabel"; import { alertClasses } from "@mui/material/Alert"; -import { TABLE_WIDTH } from "../../style"; +import { RIGHT_PANEL_WIDTH } from "../../../../style"; + +const TABLE_WIDTH = RIGHT_PANEL_WIDTH - 60; export const TableWrapper = styled.div` - width: ${TABLE_WIDTH}; + width: ${TABLE_WIDTH}px; [class*="StyledCell"] { - max-width: ${parseFloat(TABLE_WIDTH.replace("px", "")) / 3}px; + max-width: ${TABLE_WIDTH / 3}px; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; @@ -57,7 +59,7 @@ export const EffectSizeHeaderWrapper = styled.div` export const CellGroupWrapper = styled.div` min-height: 92px; height: fit-content; - width: ${TABLE_WIDTH}; + width: ${TABLE_WIDTH}px; margin-bottom: 8px; padding: 12px; @@ -90,7 +92,7 @@ export const FilterTagsWrapper = styled.div` `; export const StyledCallout = styled(Callout)` - width: ${TABLE_WIDTH}; + width: ${TABLE_WIDTH}px; .${alertClasses.icon} { margin-top: auto; diff --git a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/style.ts b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/style.ts index f46062dc6ee67..1e929401ee433 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/style.ts +++ b/frontend/src/views/DifferentialExpression/components/Main/components/DeResults/style.ts @@ -7,8 +7,9 @@ import { fontHeaderXl, } from "@czi-sds/components"; import { gray500, primary400 } from "src/common/theme"; +import { RIGHT_PANEL_WIDTH } from "../../style"; -export const TABLE_WIDTH = "480px"; +export const TABLE_WIDTH = RIGHT_PANEL_WIDTH - 60; export const ButtonLabel = styled.span` ${fontBodyM} @@ -23,7 +24,7 @@ export const ResultsHeaderWrapper = styled.div` flex-direction: row; margin-bottom: 16px; justify-content: space-between; - width: ${TABLE_WIDTH}; + width: ${TABLE_WIDTH}px; `; interface ButtonsWrapperProps { @@ -51,6 +52,7 @@ export const InstructionsWrapper = styled.div` max-width: 368px; margin-left: 16px; margin-top: 16px; + padding-right: 20px; `; export const InstructionsHeader = styled.div` ${fontHeaderL} diff --git a/frontend/src/views/DifferentialExpression/components/Main/components/Filters/components/FilterDropdown/connect.ts b/frontend/src/views/DifferentialExpression/components/Main/components/Filters/components/FilterDropdown/connect.ts index 3e52bc34f07c1..5b35ecc40bfdf 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/components/Filters/components/FilterDropdown/connect.ts +++ b/frontend/src/views/DifferentialExpression/components/Main/components/Filters/components/FilterDropdown/connect.ts @@ -1,5 +1,5 @@ import { DispatchContext } from "src/views/DifferentialExpression/common/store"; -import { useContext, useMemo, useState } from "react"; +import { useContext, useEffect, useMemo, useState } from "react"; import { Props } from "./types"; import { @@ -44,23 +44,18 @@ export const useConnect = ({ (a, b) => selectedOptionIds.indexOf(a.id) - selectedOptionIds.indexOf(b.id) ); - if (dispatch) { - if (isQueryGroup1) { - dispatch(setSelectedOptionsGroup1(queryGroupKey, newOptions)); - } else { - dispatch(setSelectedOptionsGroup2(queryGroupKey, newOptions)); - } - } return newOptions; - }, [ - options, - allAvailableOptions, - selectedOptionIds, - queryGroupKey, - isQueryGroup1, - dispatch, - ]); + }, [options, allAvailableOptions, selectedOptionIds]); + + useEffect(() => { + if (dispatch) { + const action = isQueryGroup1 + ? setSelectedOptionsGroup1 + : setSelectedOptionsGroup2; + dispatch(action(queryGroupKey, selectedOptions)); + } + }, [selectedOptions, dispatch, isQueryGroup1, queryGroupKey]); return { selectedOptions, diff --git a/frontend/src/views/DifferentialExpression/components/Main/components/Method/index.tsx b/frontend/src/views/DifferentialExpression/components/Main/components/Method/index.tsx index 84033402f9c7a..12626b72c29b5 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/components/Method/index.tsx +++ b/frontend/src/views/DifferentialExpression/components/Main/components/Method/index.tsx @@ -3,8 +3,9 @@ import { InputDropdownProps as RawInputDropdownProps } from "@czi-sds/components import { EMPTY_ARRAY, noop } from "src/common/constants/utils"; import { StyledDropdown, Label } from "../common/style"; -import { Wrapper } from "./style"; +import { StyledP, StyledTooltipText, Wrapper } from "./style"; import { DIFFERENTIAL_EXPRESSION_METHOD_DROPDOWN } from "src/views/DifferentialExpression/common/constants"; +import HelpTooltip from "src/views/CellGuide/components/CellGuideCard/components/common/HelpTooltip"; const InputDropdownProps: Partial = { sdsStyle: "square", @@ -16,7 +17,45 @@ export default function Method(): JSX.Element { const method = METHODS[0]; return ( - + { - setActiveValue(value); - }} + onChange={(_, value) => value && setActiveValue(value)} defaultValue="excludeTwo" buttonDefinition={BUTTON_DEFINITION} /> @@ -29,18 +27,18 @@ export default function Method(): JSX.Element { const BUTTON_DEFINITION = [ { - icon: , - value: "excludeOne", - tooltipText: "Exclude overlapping cells from group 1", + icon: , + value: "retainBoth", + tooltipText: "Retain overlapping cells in both groups", }, { - icon: , + icon: , value: "excludeTwo", - tooltipText: "Exclude overlapping cells from group 2", + tooltipText: "Remove overlapping cells from group 2", }, { - icon: , - value: "retainBoth", - tooltipText: "Retain overlapping cells in both groups", + icon: , + value: "excludeOne", + tooltipText: "Remove overlapping cells from group 1", }, ]; diff --git a/frontend/src/views/DifferentialExpression/components/Main/index.tsx b/frontend/src/views/DifferentialExpression/components/Main/index.tsx index 71b6432a8d76b..8cabeece1bb54 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/index.tsx +++ b/frontend/src/views/DifferentialExpression/components/Main/index.tsx @@ -13,6 +13,7 @@ import { Spinner, QueryGroupAndButtonWrapper, ClearAllButton, + StyledP, } from "./style"; import QueryGroupFilters from "./components/Filters"; import Organism from "./components/Organism"; @@ -61,49 +62,27 @@ export default function DifferentialExpression(): JSX.Element { - Find differentially expressed genes between custom group of cells - across the CELLxGENE data corpus. For additional help and - information, read our documentation. -
-
- This tool uses Welch's t-test to identify differentially - expressed genes between groups of cell in the CELLxGENE Census. - While the t-test performs reasonably well on individual datasets{" "} - - [1] - - - [2] - - - [3] - - , its performance on concatenated, non-integrated datasets has not - been extensively evaluated. We recommend using this tool for - preliminary investigations and following up with a more robust - method for formal analysis. Learn more about our data filtering - and normalization{" "} - track(EVENTS.DE_DOCUMENTATION_CLICKED)} - > - here - - . +

+ Find differentially expressed genes between custom group of + cells across the CELLxGENE data corpus. For additional help and + information, read our documentation. +

+ + This tool uses Welch's t-test to identify differentially + expressed genes between groups of cells in the CELLxGENE Census. + We recommend using this tool for preliminary investigations and + following up with more robust methods for formal analysis. Learn + more about data filtering and normalization{" "} + track(EVENTS.DE_DOCUMENTATION_CLICKED)} + > + here + + . + diff --git a/frontend/src/views/DifferentialExpression/components/Main/style.ts b/frontend/src/views/DifferentialExpression/components/Main/style.ts index 5784369178524..fb9f0d639fa3b 100644 --- a/frontend/src/views/DifferentialExpression/components/Main/style.ts +++ b/frontend/src/views/DifferentialExpression/components/Main/style.ts @@ -10,7 +10,7 @@ import { import { gray500, primary400 } from "src/common/theme"; import { SvgIcon } from "@mui/material"; -const RIGHT_PANEL_WIDTH = "480px"; +export const RIGHT_PANEL_WIDTH = 480; export const TwoPanelLayout = styled.div` display: flex; @@ -25,8 +25,8 @@ export const TwoPanelLayout = styled.div` } .rightPanel { - width: ${RIGHT_PANEL_WIDTH}; min-width: fit-content; + width: ${RIGHT_PANEL_WIDTH}px; } `; @@ -139,3 +139,7 @@ export const StyledSvgIcon = styled(SvgIcon)` color: ${primary400}; } `; + +export const StyledP = styled.p` + margin-bottom: 0; +`; diff --git a/frontend/tests/features/differentialExpression/differentialExpression.test.ts b/frontend/tests/features/differentialExpression/differentialExpression.test.ts index 8ea5fb40ff63c..ad8b00cc52a64 100644 --- a/frontend/tests/features/differentialExpression/differentialExpression.test.ts +++ b/frontend/tests/features/differentialExpression/differentialExpression.test.ts @@ -789,7 +789,7 @@ describe("Differential Expression", () => { // Ensure the callout is visible when overlapping cells are not filtered await page .getByTestId(DIFFERENTIAL_EXPRESSION_OVERLAP_BEHAVIOR) - .locator("button:nth-child(3)") + .locator("button:nth-child(1)") .click(); await isElementVisible(page, DIFFERENTIAL_EXPRESSION_RESULTS_CALLOUT); }); @@ -893,6 +893,11 @@ const runDEQuery = async ({ cellTypeFilterAutocompleteGroup2, "acinar cell" ); + } else { + await page + .getByTestId(DIFFERENTIAL_EXPRESSION_OVERLAP_BEHAVIOR) + .locator("button:nth-child(2)") + .click(); } if (mode === "test_exclude_unavailable_tags") { // Type "acinar cell" in cell type filter for group 1