Mutation Plot Diagram on Study View - GSoC #4646

aditygrg2 · 2023-06-20T17:48:28Z

Mutation Plot on Study View

onursumer · 2023-06-20T18:45:47Z

packages/react-mutation-mapper/src/component/lollipopPlot/LollipopPlotNoTooltip.tsx

+            const filteredDataForStudyView = filteredDataByCodon(
+                codon,
+                this.props.dataStore
+            );
+
+            // Send the data to study view if the page is study view.
+            if (this.props.getFilteredData !== undefined) {
+                this.props.getFilteredData(filteredDataForStudyView);
+            }


This is a low level library component. We should not introduce anything study view specific here. This needs to be done outside of this component. Also, I don't think we need a new filteredDataByCodon function. It should be already computed in the store. This is probably what we should be using instead:

cbioportal-frontend/packages/react-mutation-mapper/src/store/DefaultMutationMapperStore.ts

Line 308 in 4a7f358

public get mutationsByPosition(): { [pos: number]: T[] } {

Thank you for the suggestions, the new implementation now uses this method.

onursumer · 2023-06-20T18:51:21Z

packages/react-mutation-mapper/src/component/lollipopPlot/LollipopPlot.tsx

@@ -37,6 +37,7 @@ export type LollipopPlotProps = {
    dataStore?: DataStore;
    onXAxisOffset?: (offset: number) => void;
    yAxisLabelFormatter?: (symbol?: string, groupName?: string) => string;
+    getFilteredData?: (selectedLollipopData: any[]) => void;


I don't think we need this getFilteredData property in this component.

This is removed now.

onursumer · 2023-06-20T18:55:07Z

packages/react-mutation-mapper/src/component/lollipopMutationPlot/LollipopMutationPlot.tsx

+    public setCustomDataByCodon(data: any[]) {
+        this.props.store.sampleDataByCodon = data.slice();
+    }


This is not the correct way to do it. We don't need an action like this. Instead sampleDataByCodon can and should be computed inside the store.

This is removed now.

onursumer · 2023-06-20T18:59:49Z

packages/react-mutation-mapper/src/store/DefaultMutationMapperStore.ts

+    clearFilters() {
+        this.sampleDataByCodon = [];
+    }


This is pretty confusing, and most likely we don't even need a function like this. sampleDataByCodon should be a computed value.

Yes, I agree. This is removed too. Now there is StudyViewMutationMapperStore with computed samplesViaSelectedCodons

onursumer · 2023-06-20T19:03:42Z

packages/react-mutation-mapper/src/store/DefaultMutationMapperStore.ts

@@ -106,6 +106,7 @@ class DefaultMutationMapperStore<T extends Mutation>

    @observable
    private _selectedTranscript: string | undefined = undefined;
+    sampleDataByCodon?: any[] = [];


This should be a computed value. Also should be a well defined type, not just any. We may need to do this in a more specific store, like StudyViewMutationMapperStore.

The type is updated in the new implementation.

onursumer · 2023-06-20T19:09:28Z

packages/react-mutation-mapper/src/util/FilterUtils.ts

@@ -17,6 +17,26 @@ import { ApplyFilterFn } from '../model/FilterApplier';

 export const TEXT_INPUT_FILTER_ID = '_mutationTableTextInputFilter_';

+export function filteredDataByCodon(


We don't need to recompute filtered data here. This data already exists in the mutation mapper store. We just need a @computed value to get samplesByPosition.

Another thing is the sample defined in this function is actually a Partial<Mutation>, we can probably use mutationsByPosition as is without even adding a new function.

This function is removed now.

onursumer · 2023-06-20T19:10:50Z

src/pages/studyView/StudyViewPageStore.ts

@@ -2647,6 +2739,7 @@ export class StudyViewPageStore
        this.submittedPillStore = {};
        this.hesitantPillStore = {};
        this.resetClinicalEventTypeFilter();
+        this.mutationPlotStore[this.selectedMutationPlotGene].clearFilters();


Let's just remove this

onursumer · 2023-06-20T19:26:19Z

src/pages/studyView/charts/ChartContainer.tsx

@@ -175,6 +181,7 @@ export interface IChartContainerProps {
 @observer
 export class ChartContainer extends React.Component<IChartContainerProps, {}> {
    private chartHeaderHeight = 20;
+    private mutationPlotRef: any;


we should avoid any and be more specific here.

Updated with the component name.

onursumer · 2023-06-20T19:26:36Z

src/pages/studyView/charts/ChartContainer.tsx

@@ -192,6 +199,7 @@ export class ChartContainer extends React.Component<IChartContainerProps, {}> {
        super(props);

        makeObservable(this);
+        this.mutationPlotRef = React.createRef();


We probably don't need this line

Yes, I was just trying out different things, removed this line

…rtal-frontend into mutation-mapper

…tend

…Store

onursumer · 2023-07-04T19:39:17Z

packages/cbioportal-utils/src/model/Mutation.ts

+    patientId?: string;
+    studyId?: string;
+    sampleId?: string;


we should not modify our base Mutation model. These fields are specific to cbioportal mutation data. This mutation model is designed to be generic.

Updated this model

onursumer · 2023-07-04T19:41:55Z

packages/react-mutation-mapper/src/model/MutationMapperStore.ts

@@ -83,6 +83,7 @@ export interface MutationMapperStore<T extends Mutation> {
    getTranscriptId?: () => string | undefined;
    selectedTranscript?: string | undefined;
    ptmSources?: string[];
+    sampleDataByCodon?: any[];


This should probably be defined somewhere else mutation mapper store should not store sample/patient data. Also we should avoid any type

onursumer · 2023-07-04T19:43:07Z

src/pages/studyView/StudyViewPageStore.ts

@@ -1,4 +1,4 @@
-import _ from 'lodash';
+import _, { uniq } from 'lodash';


we don't need this import, we can simply call _.uniq

onursumer · 2023-07-04T19:45:19Z

src/pages/studyView/StudyViewPageStore.ts

+        if (selectedTable == FreqColumnTypeEnum.MUTATION)
+            this.addMutationPlot(hugoGeneSymbol);


Suggested change

if (selectedTable == FreqColumnTypeEnum.MUTATION)

this.addMutationPlot(hugoGeneSymbol);

if (selectedTable == FreqColumnTypeEnum.MUTATION) {

this.addMutationPlot(hugoGeneSymbol);

}

onursumer · 2023-07-04T19:45:39Z

src/pages/studyView/StudyViewPageStore.ts

+    @action.bound
+    addMutationPlot(hugoGeneSymbol: string): void {
+        if (!this.visibleMutationPlotByGene(hugoGeneSymbol)) {
+            let mutationPlotSpecs = [


Suggested change

let mutationPlotSpecs = [

const mutationPlotSpecs = [

onursumer · 2023-07-04T20:31:32Z

src/pages/studyView/charts/ChartContainer.tsx

+                    toggleXAxisAtTop={
+                        this.chartType === ChartTypeEnum.MUTATION_DIAGRAM
+                            ? this.props.store.getMutationStore(
+                                  this.props.chartMeta.uniqueKey
+                              ).toggleXAxisOnTop
+                            : undefined
+                    }


again, probably we won't need this

onursumer · 2023-07-04T20:33:16Z

src/pages/studyView/charts/mutationPlot/StudyViewMutationMapperStore.ts

@@ -0,0 +1,66 @@
+import { Mutation } from 'cbioportal-ts-api-client';
+import { DefaultMutationMapperStoreConfig } from '../../../../../packages/react-mutation-mapper/dist/store/DefaultMutationMapperStore';


This is not the right way to import from packages. Do this instead:

Suggested change

import { DefaultMutationMapperStoreConfig } from '../../../../../packages/react-mutation-mapper/dist/store/DefaultMutationMapperStore';

import { DefaultMutationMapperStoreConfig } from 'react-mutation-mapper';

onursumer · 2023-07-04T20:33:54Z

src/pages/studyView/charts/mutationPlot/StudyViewMutationMapperStore.ts

+    }
+
+    @computed
+    get samplesViaSelectedCodons(): SampleData[] {


samplesByPosition

onursumer · 2023-07-04T20:48:21Z

src/pages/studyView/charts/mutationPlot/StudyViewMutationMapperStore.ts

+        const selectedPositions = dataStore.selectedPositions;
+        const mutationsByPosition = this.mutationsByPosition;
+
+        const mutationData = Object.values(selectedPositions).map(


We should be more clear here. This should be mutationsGroupedBySelectedPostions, right?

onursumer · 2023-07-04T20:52:53Z

src/pages/studyView/charts/mutationPlot/StudyViewMutationMapperStore.ts

+        const samplesViaSelectedCodons: SampleData[] = [];
+
+        mutationData.map(val => {
+            val.map(m => {
+                samplesViaSelectedCodons.push({
+                    patientId: m.patientId,
+                    studyId: m.studyId,
+                    sampleId: m.sampleId,
+                    value: m.keyword,
+                });
+            });
+        });
+
+        return samplesViaSelectedCodons;


we are not using the return valiue of mutationData.map here, ideally we should be doing something like this:

Suggested change

const samplesViaSelectedCodons: SampleData[] = [];

mutationData.map(val => {

val.map(m => {

samplesViaSelectedCodons.push({

patientId: m.patientId,

studyId: m.studyId,

sampleId: m.sampleId,

value: m.keyword,

});

});

});

return samplesViaSelectedCodons;

return _.flatten(mutationData).map(m =>

({

patientId: m.patientId,

studyId: m.studyId,

sampleId: m.sampleId,

value: m.keyword,

})

);

…tend

onursumer · 2023-08-24T13:32:25Z

src/pages/studyView/StudyViewPage.tsx

@@ -178,6 +178,10 @@ export default class StudyViewPage extends React.Component<
        let hashString: string = hash || getBrowserWindow().studyPageFilter;
        delete (window as any).studyPageFilter;

+        this.store.mutationMapperFF = query['test']


is this a placeholder for a feature flag?

onursumer · 2023-08-24T13:32:48Z

src/pages/studyView/StudyViewPageStore.ts

@@ -358,6 +375,7 @@ export type StudyViewURLQuery = {
    filterValues?: string;
    sharedGroups?: string;
    sharedCustomData?: string;
+    test?: string;


placeholder for feature flag?

onursumer · 2023-08-24T13:38:37Z

src/pages/studyView/StudyViewPageStore.ts

+    readonly genes = remoteData<Gene[]>({
+        await: () => [],
+        invoke: async () => {
+            let geneData: Gene[] = [];


no need to define a variable, we can just return the value from the map function.

onursumer · 2023-08-24T13:39:15Z

src/pages/studyView/StudyViewPageStore.ts

+        invoke: async () => {
+            let mutations: Mutation[] = [];
+
+            this.visibleMutationPlotGenes.map(gene => {


we should use the value returned by map function instead of defining a redundant mutations variable

onursumer · 2023-08-24T13:40:19Z

src/pages/studyView/StudyViewPageStore.ts

@@ -2568,8 +2601,70 @@ export class StudyViewPageStore
        );
    }

+    mutationMapperFF = true;


we should be more clear with variable naming, something like mutationMapperFeatureEnabled would be better

onursumer · 2023-08-24T14:21:24Z

src/pages/studyView/chartHeader/ChartHeader.tsx

@@ -80,6 +86,7 @@ export interface ChartControls {
    isShowNAChecked?: boolean;
    showNAToggle?: boolean;
    showSwapAxes?: boolean;
+    showResultsPageButton?: boolean;


let's make it more clear, something like

Suggested change

showResultsPageButton?: boolean;

showMutationDiagramResultsPageButton?: boolean;

onursumer · 2023-08-24T15:19:44Z

src/pages/studyView/charts/mutationPlot/StudyViewMutationMapperStore.ts

+    value: string;
+};
+
+interface DefaultMutationMapperStoreConfig {


do we really need to duplicate DefaultMutationMapperStoreConfig? We can probably reuse the existing one.

Here, the package does not have DefaultMutationMapperStoreConfig exported, so needed to create a copy on our frontend app.

We can export DefaultMutationMapperStoreConfig from the package's index.tsx. We already made some modifications to the package itself, so it should okay to modify the index.tsx as well.

onursumer · 2023-08-24T15:22:36Z

src/pages/studyView/table/GeneCell.tsx

+        tableType: FreqColumnTypeEnum
+    ) => void;
+    selectedMutationPlotGenes?: string[];
+    mutationMapperFF?: boolean;


we need to name this property better. something like enableMutationDiagramButton

onursumer · 2023-08-24T15:23:15Z

src/pages/studyView/table/MultiSelectionTable.tsx

@@ -95,6 +101,9 @@ export type MultiSelectionTableProps = BaseMultiSelectionTableProps & {
    selectedGenes: string[];
    onGeneSelect: (hugoGeneSymbol: string) => void;
    columns: MultiSelectionTableColumn[];
+    setOperationsButtonText: string;
+    selectedMutationPlotGenes?: string[];
+    mutationMapperFF?: boolean;


same here, we need a better name

onursumer · 2023-08-24T15:24:02Z

src/routes.tsx

@@ -377,7 +377,7 @@ export const makeRoutes = () => {
                    })}
                />
                <Route
-                    path="/results/:tab?"
+                    path="/results/:tab"


why do we need to modify this file?

Hi, this is a mistake. I will revert this.

…rtal-frontend into mutation-mapper

aditygrg2 added 8 commits June 5, 2023 15:08

click to add mutation diagram box on study view

a20ab35

chart updated with backend lollipop data

a384f37

more improvements to the plot

0ee954f

Merge branch 'master' into mutation-mapper

5d6809a

Merge branch 'cBioPortal:master' into mutation-mapper

c735090

updated the plot

ea6f20e

Merge branch 'cBioPortal:master' into mutation-mapper

6a13c48

updated the plot

390918d

aditygrg2 marked this pull request as draft June 20, 2023 17:49

Merge branch 'mutation-mapper' of https://github.com/aditygrg2/cbiopo…

414a491

…rtal-frontend into mutation-mapper

onursumer requested changes Jun 20, 2023

View reviewed changes

changes to the filtering implementation

59e18c5

aditygrg2 requested a review from onursumer June 21, 2023 20:35

aditygrg2 added 14 commits June 24, 2023 10:38

Merge branch 'master' into mutation-mapper

be0dec2

multiple charts and initial filter design

4e9c7c7

Merge branch 'mutation-mapper' of https://github.com/aditygrg2/cbiopo…

3d4da75

…rtal-frontend into mutation-mapper

Minor bug fixes

c93894b

minor bug fixes, update to resize

96ca029

minor bug fixes, drag bug fix, filter button is now absolute

bbcd53e

Fixed chart filter issue at the top

ef813bb

Merge branch 'master' into mutation-mapper

7b0b685

Merge branch 'master' into mutation-mapper

8f450b3

added stacked filtering feature

309e6b4

added a new route

994eff8

Merge branch 'master' of https://github.com/aditygrg2/cbioportal-fron…

e196a1d

…tend

Merge branch 'cBioPortal:master' into master

3a9fe9d

fixed a bug where all charts can be created a studyViewMutationMapper…

6c10d19

…Store

onursumer requested changes Jul 4, 2023

View reviewed changes

aditygrg2 added 2 commits July 9, 2023 14:10

code refactoring

75f62b8

Merge branch 'master' into mutation-mapper

e39c1b2

aditygrg2 added 12 commits July 14, 2023 19:45

Merge branch 'master' into mutation-mapper

56ce6ef

Removed xAxisTop Feature

5c245bc

saving some changes related to addition of new features

a32a374

mutation-mapper commit

26401b0

changes with routes

64733df

Merge branch 'master' of https://github.com/aditygrg2/cbioportal-fron…

5155dcf

…tend

Merge branch 'master' into master

6a60b43

usabiliry improvements to mutation plot

2bc986f

enabled tooltip and multi-annotated plots

8d96414

code refactor

936acda

Merge branch 'master' into mutation-mapper

9250424

Merge branch 'cBioPortal:master' into mutation-mapper

67a84e4

onursumer requested changes Aug 24, 2023

View reviewed changes

aditygrg2 added 5 commits August 31, 2023 00:15

updates and refactoring

d974ca3

updates and refactoring

b9fa781

updates and refactoring

0a08107

Merge branch 'master' into mutation-mapper

080cf17

Merge branch 'master' into mutation-mapper

c35a7a8

aditygrg2 changed the title ~~Mutation Plot Diagram on Study View - Draft PR~~ Mutation Plot Diagram on Study View - GSoC Sep 3, 2023

aditygrg2 added 6 commits September 3, 2023 12:20

removal of xAxisOnTop

200f2a7

Merge branch 'mutation-mapper' of https://github.com/aditygrg2/cbiopo…

4b6ff4b

…rtal-frontend into mutation-mapper

fix routes.tsx file

a2094a6

Merge branch 'master' into mutation-mapper

1a39226

loading issue with annotated mutation plots

d2746f1

Merge branch 'master' into mutation-mapper

d2787a2

		@@ -17,6 +17,26 @@ import { ApplyFilterFn } from '../model/FilterApplier';

		export const TEXT_INPUT_FILTER_ID = '_mutationTableTextInputFilter_';

		export function filteredDataByCodon(

		@@ -1,4 +1,4 @@
		import _ from 'lodash';
		import _, { uniq } from 'lodash';

		if (selectedTable == FreqColumnTypeEnum.MUTATION)
		this.addMutationPlot(hugoGeneSymbol);

		@@ -0,0 +1,66 @@
		import { Mutation } from 'cbioportal-ts-api-client';
		import { DefaultMutationMapperStoreConfig } from '../../../../../packages/react-mutation-mapper/dist/store/DefaultMutationMapperStore';

	showResultsPageButton?: boolean;
	showMutationDiagramResultsPageButton?: boolean;

Mutation Plot Diagram on Study View - GSoC #4646

Are you sure you want to change the base?

Mutation Plot Diagram on Study View - GSoC #4646

Conversation

aditygrg2 commented Jun 20, 2023 • edited Loading

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

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

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

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

aditygrg2 commented Jun 20, 2023 •

edited

Loading