Genie fusion issue 5613 #2311

khzhu · 2019-05-13T18:23:20Z

What? Why?

Fix #

Decouple fusions from mutations should fix the issue.

Changes proposed in this pull request:

add a new Fusion table without Frequency column in the study view to show all fusions currently recorded in the mutation database table.

Checks

Is this PR adding logic based on one or more clinical attributes? If yes, please make sure validation for this attribute is also present in the data validation / data loading layers (in backend repo) and documented in File-Formats Clinical data section!
Follows 7 rules of great commit messages. For most PRs a single commit should suffice, in some cases multiple topical commits can be useful. During review it is ok to see tiny commits (e.g. Fix reviewer comments), but right before the code gets merged to master or rc branch, any such commits should be squashed since they are useless to the other developers. Definitely avoid merge commits, use rebase instead.
Follows the Airbnb React/JSX Style guide.
Make sure your commit messages end with a Signed-off-by string (this line
can be automatically added by git if you run the git-commit command with
the -s option)

Any screenshots or GIFs?

If this is a new visual feature please add a before/after screenshot or gif
here with e.g. GifGrabber.

Notify reviewers

Read our Pull request merging
policy. If you are part of the
cBioPortal organization, notify the approprate team (remove inappropriate):

@cBioPortal/frontend

If you are not part of the cBioPortal organization look at who worked on the
file before you. Please use git blame <filename> to determine that
and notify them here:

alisman · 2019-06-21T15:13:33Z

src/shared/components/lollipopMutationPlot/LollipopMutationPlot.tsx

@@ -271,8 +271,8 @@ export default class LollipopMutationPlot extends React.Component<ILollipopMutat

    @computed private get domainMap(): {[pfamAccession:string]: PfamDomain}
    {
-        if (!this.props.store.pfamDomainData.isPending && 
-            this.props.store.pfamDomainData.result && 
+        if (!this.props.store.pfamDomainData.isPending &&


lets revert this

alisman · 2019-06-21T15:15:57Z

src/pages/studyView/StudyViewPageStore.ts

@@ -1054,6 +1056,9 @@ export class StudyViewPageStore {
                case ChartTypeEnum.MUTATED_GENES_TABLE:
                    this.resetGeneFilter();
                    break;
+                case ChartTypeEnum.FUSION_GENES_TABLE:
+                    this.resetGeneFilter();


just confirming that we don't have separate FusionGene filter?

we do not. Fusions are not properly tracked in the database at the moment. This is really a temporary solution to address GENIE study mutations over counting problem.

update: discussed with @zhx828, added a new gene filter for fusions.

this should be resetFusionGeneFilter right?

alisman · 2019-06-21T15:21:23Z

src/pages/studyView/StudyViewPageStore.ts

+    readonly fusionGeneData = remoteData<FusionGenesData>({
+        await: () => [this.mutationProfiles],
+        invoke: async () => {
+            if (!_.isEmpty(this.mutationProfiles.result!)) {


not too important, but you might not have to use ! after result here. one would think that isEmpty typing would accept undefined since it's meant to test for undefined (among other things).

I see. thanks for pointing this out. Will remove !.

alisman · 2019-06-21T15:22:06Z

src/pages/studyView/StudyViewPageStore.ts

+        await: () => [this.mutationProfiles],
+        invoke: async () => {
+            if (!_.isEmpty(this.mutationProfiles.result!)) {
+                // TODO: get data for all profiles


not sure if i understand the TODO

sorry, I forgot too. It has been really a while, will back to you later.

I will remove these TODOs. I placed there as a reminder for a feature which should be transferred to a proper issue on github.

alisman · 2019-06-21T15:40:54Z

src/pages/studyView/table/FusionGenesTable.tsx

+class FusionGenesTableComponent extends FixedHeaderTable<MutationCountByGene> {
+}
+
+@observer


@khzhu @zhx828 Just for discusion but do you think there's opportunity for reusing more code (super class?) between these different table types? i would say we should do it only if it's obvious that we're copying and pasting a lot. Or if there are going to be a lot more table types.

@JJ, @alisman, @zhx828,
how will fusion table be displayed once structure variant PR merged in, will still be as same as mutation table?
I am re-thinking how to address Aaron's above comment to factor gene tables. CNAGenesTable and MutationGenesTable are a bit different since we have cytobands and alterations in the CNAGenesTable, while have Frequencies in the MutationGenesTable, makes sense to give them their own tables. But new fusionGenesTable is merely a copy of Mutation table, since it does not have its own data model at the moment. If FusionGenesTable is always as same as MutationGenesTable, it will make more sense to have a super class for them.

@khzhu we may keep it the same way after using the SV table for the first iteration. In the long run, we may want to to find a way to display the partner genes, but let's not worry about it for now.

thanks, @JJ.
I vote for having FusionGenesTable by its own for the future so we could easily add partner genes and possible remove Freq coloumn(do not think is useful). What do you think @alisman?

here are three tables side by side

@alisman these tables are using the same component FixedHeaderTable. There is no much of space to create a super class for these study view tables since they are highly customized. But I do think there are more space to move some of the functions to utils so they can be shared.

moved some common interface/type to TabUtil file for share.

@alisman I merged three tables to one component. It's not a super class, since we already using FixedHeaderTable. Essentially, it's just a util component including all the column definitions.

alisman · 2019-06-21T15:41:42Z

src/pages/studyView/table/FusionGenesTable.tsx

+
+    @autobind
+    @action
+    updateCellMargin() {


this seems like it could be shared b/c there's nothing implementation specific. or is there?

no longer use updateCellMargin().

zhx828 · 2019-08-28T16:18:21Z

src/pages/studyView/StudyViewPageStore.ts

@@ -2743,13 +2829,40 @@ export class StudyViewPageStore {
        default: []
    });

+    // @ts-ignore


Not sure why Kelsey added this. I'm looking into

kalletlak · 2019-08-29T18:22:42Z

src/pages/studyView/StudyViewConfig.ts

@@ -77,6 +78,7 @@ export enum ChartTypeNameEnum {
    TABLE = 'table',
    SCATTER = 'density plot',
    MUTATED_GENES_TABLE = 'table',
+    FUSION_GENES_TABLE = 'table',


Reminder to me - Need to update study page setting api to support FUSION_GENES_TABLE

Updated in https://github.com/cBioPortal/cbioportal/pull/6093/commits

kalletlak · 2019-08-29T18:27:32Z

src/pages/studyView/StudyViewPageStore.ts

@@ -1054,6 +1056,9 @@ export class StudyViewPageStore {
                case ChartTypeEnum.MUTATED_GENES_TABLE:
                    this.resetGeneFilter();
                    break;
+                case ChartTypeEnum.FUSION_GENES_TABLE:
+                    this.resetGeneFilter();


this should be resetFusionGeneFilter right?

kalletlak · 2019-08-29T18:28:06Z

src/pages/studyView/StudyViewPageStore.ts

@@ -1236,6 +1292,9 @@ export class StudyViewPageStore {
                case ChartTypeEnum.MUTATED_GENES_TABLE:
                    this.resetGeneFilter();


may be rename it to resetMutatedGeneFilter

kalletlak · 2019-08-29T18:50:30Z

src/pages/studyView/TableUtils.tsx

+            <Else>
+                <DefaultTooltip
+                    placement="right"
+                    disabled={type === 'fusion'}


this won't be rendered when type === 'fusion'

kalletlak · 2019-08-29T19:00:06Z

src/pages/studyView/TableUtils.tsx

+}
+
+export function rowIsChecked(entrezGeneId:number, preSelectedRows:AlteredGenesTableUserSelectionWithIndex[], selectedRows:AlteredGenesTableUserSelectionWithIndex[]) {
+    const record = _.find(


why not do find in preSelectedRows and selectedRows together?

kalletlak · 2019-08-29T19:04:29Z

src/pages/studyView/TableUtils.tsx

+}
+
+export function rowIsDisabled(entrezGeneId: number, selectedRows:AlteredGenesTableUserSelectionWithIndex[]) {
+    return !_.isUndefined(


may be !!_.find(... instead of !_.isUndefined(

alisman · 2019-10-07T16:24:18Z

src/pages/studyView/table/GenePanelModal.tsx

+                </Modal>
+            );
+        } else {
+            return <span />;


kalletlak · 2019-10-08T14:04:48Z

src/pages/studyView/table/GeneTable.tsx

+
+    @computed
+    get tableColumns() {
+        return _.reduce(this.props.columns, (acc, column) => {


why not use map

alisman · 2019-10-09T15:20:35Z

src/pages/studyView/StudyViewPageStore.ts

@@ -768,6 +769,18 @@ export class StudyViewPageStore {
            }, [] as MutationGeneFilter[]);
        }

+        if (_.isArray(filters.fusionGenes) && filters.fusionGenes.length > 0) {
+            this._fusionGeneFilter = _.reduce(filters.fusionGenes, (acc, next) => {


i can see that this logic follows pattern of existing blocks in use of _.reduce, but worth noting that these reduces are really mapping operations. would be more semantic to use map. reduce should only be used if there's a conditional inclusion (filter would also work) or if we're producing a map from an array. also, we can now use native reduce, map, filter methods of array object. lodash unnecessary in these cases.

@alisman Won't the map alter the original array?

Sorry, it was for another scenario. In this case, it will not. Will update. Thanks.

alisman · 2019-10-09T15:26:45Z

src/pages/studyView/StudyViewPageStore.ts

+
+    @autobind
+    @action
+    removeFusionGeneFilter(toBeRemoved: number) {


toBeRemoved = entrezIdToBeRemoved

alisman · 2019-10-09T15:32:03Z

src/pages/studyView/StudyViewPageStore.ts

@@ -2397,6 +2470,18 @@ export class StudyViewPageStore {
            };
        }

+        if (!_.isEmpty(this.mutationProfiles.result)) {


just want to double check that this is the right condition for including fusion table? (mutationProfiles not empty which is used by MUTATED_GENES_TABLE as well)

@alisman , that is only way but not a right way, I totally agree. We do not have a dedicate profiling type for fusions at the moment, since fusions are currently treated as one type of mutations and recorded in the mutation table. This will eventually be fixed once we move fusions out from the mutation table and stored in the structure variants table. Fusions are indeed a type of structure variants, when two genes fussed and formed a new gene. Hope this makes sense.

alisman · 2019-10-09T15:33:07Z

src/pages/studyView/StudyViewPageStore.ts

@@ -2602,6 +2689,8 @@ export class StudyViewPageStore {

            if (chartUserSettings.chartType === UniqueKey.MUTATED_GENES_TABLE) {
                this._filterMutatedGenesTableByCancerGenes = chartUserSettings.filterByCancerGenes === undefined ? true : chartUserSettings.filterByCancerGenes;
+            } else if (chartUserSettings.chartType === UniqueKey.FUSION_GENES_TABLE) {


seems like this if else if else structure should really be a switch statement.

alisman · 2019-10-09T15:43:10Z

src/pages/studyView/StudyViewPageStore.ts

                let mutatedGenes = await internalClient.fetchMutatedGenesUsingPOST({
                    studyViewFilter: this.filters
                });
                return mutatedGenes.map(item => {
                    return {
                        ...item,
+                        uniqueKey: getMutationUniqueKey(item.entrezGeneId, item.hugoGeneSymbol),
+                        oncokbAnnotated: this.oncokbCancerGeneFilterEnabled ? this.oncokbAnnotatedGeneEntrezGeneIds.result.includes(item.entrezGeneId) : false,
+                        oncokbOncogene: this.oncokbCancerGeneFilterEnabled ? this.oncokbOncogeneEntrezGeneIds.result.includes(item.entrezGeneId) : false,


if these values are boolean, i think the names should reflect it. e.g. isOncoKBOncogene

alisman · 2019-10-09T15:45:27Z

src/pages/studyView/StudyViewPageStore.ts

@@ -3023,19 +3148,19 @@ export class StudyViewPageStore {
        default: []
    });

-    readonly cnaGeneData = remoteData<CopyNumberCountByGeneWithCancerGene[]>({
+    readonly cnaGeneData = remoteData<GeneTableRow[]>({


Is this a TableRow in the sense of a database row or a table row? If the latter, i think name of the promise (cnaGeneData) should reflect that. "cnaGeneTableRowData"

alisman · 2019-10-09T16:03:16Z

src/pages/studyView/TableUtils.tsx

            />
        </span>
    );
+
+    function getCellContent() {


kinda strange pattern here. uses style variable from closure. i would rather just see this inside of the markup as it was

alisman · 2019-10-09T16:07:02Z

src/pages/studyView/chartHeader/ChartHeader.tsx

@@ -40,7 +40,7 @@ export interface IChartHeaderProps {
    chartControls?   : ChartControls;
    changeChartType  : (chartType: ChartType) => void;
    getSVG?          : ()=>Promise<SVGElement | null>;
-    getData?         : (dataType?:DataType)=>Promise<string | null>;
+    getData?         : ((dataType?:DataType)=>Promise<string | null>) | ((dataType?:DataType)=>string);


i'm curious why? seems like code will now have to check whether it's a promise or not, no?

Not sure what's the best way here. There are some getData methods don't require a promise to produce the data. If we don't provide the second type, then these methods have to attach an async tag.

alisman · 2019-10-09T16:10:16Z

src/pages/studyView/table/GenePanelModal.tsx

-                </Modal.Footer>
-            </Modal>
-        );
+        if (this.props.genePanelCache) {


seems more natural to do this outside of the modal.

I will update. I guess the original implementer was trying to avoid additional checks which have been used in three components. But now we merge these three together, so we should do the check at the place where initialize the component.

alisman

Minor comments

…s table to study view address code review comments Include fusion filter in the UserSelection & minor changes - Assign priority 85 to Fusion Genes table - Fusion Genes table does not need to show panel info Add a test to cover the fusion filter Include CNA cancer gene toggle in session service Add gene panel tooltip for fusions table & Save fusion cancer gene filter in user setting Consolidate three tables to one component which share most of the functionality Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org> Fetch genes info when the filter from url that not in the cache

zhx828 · 2019-10-12T16:27:04Z

The two coexpression tests on master env just impossible to finish. Need to double check before merging back to master

Genie fusion issue 5613

khzhu force-pushed the GENIE-fusion-issue-5613 branch from 3da8ed2 to e30419a Compare May 13, 2019 19:49

khzhu requested review from zhx828 and alisman May 13, 2019 19:52

khzhu force-pushed the GENIE-fusion-issue-5613 branch from e30419a to 470665b Compare May 14, 2019 21:21

alisman reviewed Jun 21, 2019

View reviewed changes

zhx828 added DO NOT MERGE do not review labels Jul 23, 2019

khzhu force-pushed the GENIE-fusion-issue-5613 branch from 470665b to 8bd80f8 Compare July 23, 2019 21:09

khzhu force-pushed the GENIE-fusion-issue-5613 branch 5 times, most recently from 9ccc847 to 19d0ab8 Compare August 25, 2019 05:25

khzhu force-pushed the GENIE-fusion-issue-5613 branch from 19d0ab8 to 8b5e6bd Compare August 27, 2019 19:26

zhx828 removed the do not review label Aug 27, 2019

zhx828 requested review from kalletlak and alisman and removed request for zhx828 August 28, 2019 15:34

zhx828 reviewed Aug 28, 2019

View reviewed changes

kalletlak reviewed Aug 29, 2019

View reviewed changes

zhx828 force-pushed the GENIE-fusion-issue-5613 branch 3 times, most recently from 3db6445 to 84702e7 Compare October 2, 2019 15:56

zhx828 requested a review from kalletlak October 3, 2019 19:51

zhx828 force-pushed the GENIE-fusion-issue-5613 branch from a8af10d to bda73f3 Compare October 3, 2019 19:56

alisman reviewed Oct 7, 2019

View reviewed changes

src/pages/studyView/table/GenePanelModal.tsx Outdated

</Modal>

);

} else {

return <span />;

Copy link

Collaborator

alisman Oct 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null?

zhx828 force-pushed the GENIE-fusion-issue-5613 branch 2 times, most recently from b2467cb to 088741e Compare October 7, 2019 17:23

kalletlak reviewed Oct 8, 2019

View reviewed changes

alisman reviewed Oct 9, 2019

View reviewed changes

alisman requested changes Oct 9, 2019

View reviewed changes

zhx828 force-pushed the GENIE-fusion-issue-5613 branch 3 times, most recently from b72676d to 1e63aec Compare October 11, 2019 22:39

zhx828 changed the base branch from master to release-3.1.1 October 11, 2019 22:40

zhx828 force-pushed the GENIE-fusion-issue-5613 branch 5 times, most recently from d42bd12 to 069aa63 Compare October 12, 2019 14:16

zhx828 force-pushed the GENIE-fusion-issue-5613 branch from 069aa63 to 62f1bf5 Compare October 12, 2019 15:55

zhx828 removed the DO NOT MERGE label Oct 12, 2019

zhx828 merged commit 03cff68 into cBioPortal:release-3.1.1 Oct 12, 2019

jagnathan deleted the GENIE-fusion-issue-5613 branch June 2, 2021 16:51

jagnathan pushed a commit to pughlab/cbioportal-frontend that referenced this pull request Jan 25, 2022

Merge pull request cBioPortal#2311 from pughlab/GENIE-fusion-issue-5613

5393181

Genie fusion issue 5613

		@@ -1236,6 +1292,9 @@ export class StudyViewPageStore {
		case ChartTypeEnum.MUTATED_GENES_TABLE:
		this.resetGeneFilter();

Genie fusion issue 5613 #2311

Genie fusion issue 5613 #2311

Conversation

khzhu commented May 13, 2019 • edited Loading

What? Why?

Checks

Any screenshots or GIFs?

Notify reviewers

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

zhx828 Oct 2, 2019 • 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

alisman left a comment

Choose a reason for hiding this comment

zhx828 commented Oct 12, 2019

khzhu commented May 13, 2019 •

edited

Loading

zhx828 Oct 2, 2019 •

edited

Loading