Add Total mutations as clinical variable in Plots tab #1275

oplantalech · 2018-07-05T14:21:18Z

What? Why?

Fix cBioPortal/cbioportal#4291 .

Changes proposed in this pull request:

a
b

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:

adamabeshouse

just one small change, but looks good

adamabeshouse · 2018-07-05T18:31:08Z

src/pages/resultsView/plots/PlotsTab.tsx

@@ -403,21 +403,44 @@ export default class PlotsTab extends React.Component<IPlotsTabProps,{}> {

    @computed get clinicalAttributeIdToClinicalAttribute():{[clinicalAttributeId:string]:ClinicalAttribute} {
        if (this.props.store.clinicalAttributes.isComplete) {
-            return this.props.store.clinicalAttributes.result.reduce((map:{[clinicalAttributeId:string]:ClinicalAttribute}, next)=>{
+            let _map: {[clinicalAttributeId: string]: ClinicalAttribute} = this.props.store.clinicalAttributes.result.reduce((map:{[clinicalAttributeId:string]:ClinicalAttribute}, next)=>{


you can shorten this as _.keyBy(this.props.store.clinicalAttributes.result, c=>c.clinicalAttributeId)

adamabeshouse · 2018-07-05T18:32:01Z

src/pages/resultsView/plots/PlotsTab.tsx

                map[next.clinicalAttributeId] = next;
                return map;
            }, {});
+            if (this.props.store.studyIds.isComplete) {
+                _map["total_mutations"] = {


its better to make this an enum value or something (look at the enum SpecialAttribute), at least a constant in this file const TOTAL_MUTATIONS_CLIN_ATTR_ID = "total_mutations", then reference that elsewhere below

jjgao · 2018-07-06T01:41:53Z

@adamabeshouse @oplantalech the interface improvement discussed here (cBioPortal/cbioportal#4432) may impact this PR.

oplantalech · 2018-07-06T09:24:21Z

@adamabeshouse I've implemented your suggestions and I've also rebased to rc. Is it possible to include that in the release-1.15.0 branch?

@jjgao If the changes you mention are implemented, I think they will affect very little the code I've written. In any case, though, I believe this PR will be merged before those changes are implemented.

jjgao · 2018-07-13T01:01:04Z

@oplantalech ok to point to release-1.5.0 assuming the current rc (and this pull request) does not contain other new code.

I was also asking in issue #4291 whether you would be able to add Fraction of Genome Altered as well?

jjgao · 2018-07-13T01:01:50Z

I would also like to test it before merging.

oplantalech · 2018-07-13T07:42:37Z

@jjgao Ok, I'll move it into release-1.15.0. Regarding the fraction of genome altered, it's almost ready, once this PR is merged I'm going to open a new one implementing this feature.

jjgao · 2018-07-16T15:17:12Z

Tested the feature. It looks great 👍

jjgao · 2018-07-16T15:17:33Z

Why are the e2e tests failing?

inodb · 2018-07-16T15:56:25Z

@jjgao Seems like Total Mutations is now shown in plots tab instead of Diagnosis Age attribute: https://10779-66571349-gh.circle-artifacts.com/0/imageCompare.html. Also, seems like it might be better to log scale total mutations there. Prolly outside of the scope of this pr tho

adamabeshouse · 2018-07-16T18:28:54Z

@inodb it wouldnt be very hard but yeah probably out of scope of this PR. ill log an issue: cBioPortal/cbioportal#4456

jjgao · 2018-07-16T18:56:43Z

I think we should update the screenshot and merge this pr. Log scale can be addressed in another pr.

inodb · 2018-07-16T18:58:34Z

@adamabeshouse do u know how the default clinical attribute on plots can be adjusted? Maybe better to keep using Diagnosis Age such that the plots look good. I guess it has to do with the order of clinical attributes in the dropdown menu of oncoprint?

adamabeshouse · 2018-07-16T19:08:12Z

@inodb its about the order of the clinical attributes list, not related to oncoprint

inodb · 2018-07-18T20:41:30Z

ideally test there is adjusted to select Diagnosis Age in the plots tab instead of just the first clinical attribute in the list. The total mutation count plots don't really show the whiskers very well, so kinda hard to spot regressions in that part of the code

adamabeshouse · 2018-07-18T20:50:47Z

@oplantalech you can use this command in screenshot.spec.js, in it("plots tab clinical vs molecular" between broser.click(..) and waitForAndCheckPlotsTab:

browser.execute(function() { resultsViewPlotsTab.onHorizontalAxisClinicalAttributeSelect({ value: "AGE" }); });

that should fix all the failing tests

oplantalech · 2018-07-19T08:27:50Z

@adamabeshouse I've add your command in screenshot.spec.js but the screenshots still fail

adamabeshouse · 2018-07-19T19:43:31Z

@oplantalech ah, okay so do the exact same thing in it("plots tab clinical vs clinical boxplot" and that should fix it

oplantalech added the enhancement label Jul 5, 2018

oplantalech requested review from alisman and adamabeshouse July 5, 2018 14:21

oplantalech force-pushed the total_mutations_in_plot branch from 082e37a to a0bb1d0 Compare July 5, 2018 14:24

adamabeshouse requested changes Jul 5, 2018

View reviewed changes

oplantalech force-pushed the total_mutations_in_plot branch from fff97ed to 6f518bf Compare July 6, 2018 09:20

adamabeshouse approved these changes Jul 11, 2018

View reviewed changes

oplantalech changed the base branch from rc to release-1.15.0 July 13, 2018 07:43

jjgao temporarily deployed to cbioportal-frontend-pr-1275 July 16, 2018 14:51 Inactive

oplantalech force-pushed the total_mutations_in_plot branch from 6f518bf to cb61b40 Compare July 19, 2018 07:39

jjgao temporarily deployed to cbioportal-frontend-pr-1275 July 19, 2018 07:39 Inactive

jjgao temporarily deployed to cbioportal-frontend-pr-1275 July 19, 2018 20:51 Inactive

Add total mutations as clinical variable in plots tab

b1396e3

adamabeshouse force-pushed the total_mutations_in_plot branch from 80e4193 to b1396e3 Compare July 19, 2018 21:48

jjgao deployed to cbioportal-frontend-pr-1275 July 19, 2018 21:48 View deployment

adamabeshouse merged commit 2a3cdd1 into cBioPortal:release-1.15.0 Jul 19, 2018

oplantalech deleted the total_mutations_in_plot branch July 20, 2018 08:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Total mutations as clinical variable in Plots tab #1275

Add Total mutations as clinical variable in Plots tab #1275

oplantalech commented Jul 5, 2018

adamabeshouse left a comment

adamabeshouse Jul 5, 2018

adamabeshouse Jul 5, 2018

jjgao commented Jul 6, 2018

oplantalech commented Jul 6, 2018 •

edited

Loading

jjgao commented Jul 13, 2018

jjgao commented Jul 13, 2018

oplantalech commented Jul 13, 2018

jjgao commented Jul 16, 2018

jjgao commented Jul 16, 2018

inodb commented Jul 16, 2018 •

edited

Loading

adamabeshouse commented Jul 16, 2018 •

edited

Loading

jjgao commented Jul 16, 2018

inodb commented Jul 16, 2018

adamabeshouse commented Jul 16, 2018 •

edited

Loading

inodb commented Jul 18, 2018

adamabeshouse commented Jul 18, 2018 •

edited

Loading

oplantalech commented Jul 19, 2018

adamabeshouse commented Jul 19, 2018

Add Total mutations as clinical variable in Plots tab #1275

Add Total mutations as clinical variable in Plots tab #1275

Conversation

oplantalech commented Jul 5, 2018

What? Why?

Checks

Any screenshots or GIFs?

Notify reviewers

adamabeshouse left a comment

Choose a reason for hiding this comment

adamabeshouse Jul 5, 2018

Choose a reason for hiding this comment

adamabeshouse Jul 5, 2018

Choose a reason for hiding this comment

jjgao commented Jul 6, 2018

oplantalech commented Jul 6, 2018 • edited Loading

jjgao commented Jul 13, 2018

jjgao commented Jul 13, 2018

oplantalech commented Jul 13, 2018

jjgao commented Jul 16, 2018

jjgao commented Jul 16, 2018

inodb commented Jul 16, 2018 • edited Loading

adamabeshouse commented Jul 16, 2018 • edited Loading

jjgao commented Jul 16, 2018

inodb commented Jul 16, 2018

adamabeshouse commented Jul 16, 2018 • edited Loading

inodb commented Jul 18, 2018

adamabeshouse commented Jul 18, 2018 • edited Loading

oplantalech commented Jul 19, 2018

adamabeshouse commented Jul 19, 2018

oplantalech commented Jul 6, 2018 •

edited

Loading

inodb commented Jul 16, 2018 •

edited

Loading

adamabeshouse commented Jul 16, 2018 •

edited

Loading

adamabeshouse commented Jul 16, 2018 •

edited

Loading

adamabeshouse commented Jul 18, 2018 •

edited

Loading