Display germline mutation as white stripe in oncoprint view #913

rnugraha · 2018-01-23T15:29:35Z

What? Why?

Display germline mutation as white stripe in the oncoprint track
Add background to the oncoprint rule legends

Dependency

cBioPortal/oncoprintjs#55

Checks

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?

Before

After

In zoom-out

Notify reviewers

Read our Pull request merging
policy. If you are part of the
cBioPortal organization, notify the appropriate 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:

pieterlukasse · 2018-01-25T14:28:43Z

perhaps remove all white-space changes in src/shared/components/oncoprint/geneticrules.ts...?

jjgao · 2018-01-26T20:05:32Z

@rnugraha Can we still color based on mutation types (missense, truncating, inframe) for germline mutations?

inodb · 2018-01-26T20:08:11Z

Hey thanks for submitting @rnugraha ! I don't think the white line can convey all the information. For instance: I can't see the difference between truncating germline mutation and a patient that has both a somatic truncating mutation and a germline mutatation. I think it would make more sense to make a different track for germline mutations. So in the example four tracks, two for somatic BRCA1/2 and two for germline BRCA 1/2 mutation (if there are any). That would also be useful in showing which patients had germline+somatic profiling. Thoughts @adamabeshouse @schultzn ?

adamabeshouse · 2018-01-26T20:16:31Z

just from a design perspective also, I find white is a little confusing because the other time we use white in the oncoprint is when whitespace means not sequenced, so it seems like it indicates the mutation is not as valid or important somehow (also because it makes the color of the mutation behind it quite difficult to spot)

jjgao · 2018-01-26T22:19:35Z

cc'ing @schultzn on this discussion.

Sjoerd-van-Hagen · 2018-01-29T08:51:49Z

@inodb Currently, we have one gene per track and using the OQL it can be split up into multiple. We now also hide mutations in the oncoprint if there is a mutation that 'trumps' it. I think it would be good to keep it consistent. Your suggestion is good, but would be for a different feature in my opinion: 'germline in OQL'.

pieterlukasse · 2018-01-29T09:37:32Z

@jjgao the mutation type color will be there. It is missing in the screenshot because @rnugraha disabled it from the menu when making the screenshot.

@inodb thanks for the ideas. We discussed with @schultzn in a call and I collected some of his feedback regarding the issue you raised. See last page of RFC 40.

@adamabeshouse good point. We could try a light pink color instead of white. #FFC0CB perhaps?

adamabeshouse · 2018-01-29T18:16:01Z

@pieterlukasse how about a slightly different shape? that could be another way to indicate that its still a mutation of the same type, but with an additional modifier. it might be less confusing than stacking shapes on top of each other.

What about making it a diamond instead of a square? or a square with triangles on the top and bottom to make it some kind of elongated diamond.

Sjoerd-van-Hagen · 2018-01-29T18:31:31Z

@adamabeshouse I think the problem with using a different shape is that it will not be visible when you zoom uit.

adamabeshouse · 2018-01-29T18:32:30Z

@Sjoerd-van-Hagen that's a good point, that would be a reason to favor the elongated diamond instead of a similar-sized diamond - the elongation would still be visible zoomed out

Sjoerd-van-Hagen · 2018-01-29T18:35:21Z

@adamabeshouse I am not sure what you mean or how it would remain visible when zooming out?

adamabeshouse · 2018-01-29T19:27:05Z

@Sjoerd-van-Hagen heres a mockup:

I actually agree its not that clear zoomed out. But I also don't think putting a shape in front of the square is good, because it seems to modify the graphic in a way that detracts from it, when the message should be that theres just an EXTRA piece of info about the mutation.

Maybe something like this, a little cap on top of the mutation square?
In this image is also a possibility of having a cap on top and bottom of the square

Sjoerd-van-Hagen · 2018-01-29T19:31:56Z

@adamabeshouse I would then go for a line at the top and the bottom of the square. It will still look good when zooming out and it is easier to implement.

schultzn · 2018-01-29T21:32:47Z

We won't be able to show all details. I still think the white line looks best - that could be used to indicate the presence of a germline mutation in that gene in that patient. If there are multiple mutations in the same gene and sample, the tooltip can reveal that.

jjgao · 2018-01-29T22:34:27Z

@rnugraha could you please make the white line thinner and add another screenshot here? The current white color makes the green faded away -- maybe that's part of the confusion.

Sjoerd-van-Hagen · 2018-01-30T14:27:11Z

@jjgao Riza is out of the office this week. Perhaps @adamabeshouse can do this, if it is not too much trouble?

jjgao · 2018-01-30T18:58:27Z

@Sjoerd-van-Hagen we can wait for @rnugraha.

rnugraha · 2018-02-05T12:50:04Z

Thanks all for the input and discussion. I would personally agree with the thinner white line. I shall update soon the screenshots.

rnugraha · 2018-02-08T12:04:59Z

Hi all, the PR's description and its screenshots are updated. Please beware that this PR has a dependency on cBioPortal/oncoprintjs#55.

One more thing was added:
We have a white-stripe marker on a white background. This makes the legend for germline-mutation becoming not visible. To accommodate this issue, a no-alteration rule (grey background) has been added to this marker as background. I also decided to apply it to the rest of the rule's legends to make them look more consistent.

We thought the grey background also will help other rule legends to make the positions of the markers clearer for the user.

pieterlukasse · 2018-02-08T14:26:47Z

Looks great @rnugraha , thanks for the changes! I like the grey background behind each legend item 👍

adamabeshouse

why do we need _germ in disp_mut if you're already tracking germline status with disp_germline? Looks good besides that.

adamabeshouse · 2018-03-06T15:11:22Z

@rnugraha I think there was a misunderstanding - changing disp_germline to isGermline doesn't make any difference, the issue is the redundancy between disp_germline and disp_mut. The solution I proposed was to replace every usage of disp_germline with a helper function, isGermline(d) which would check whether disp_mut contained the _germ suffix. In that case you'd have to add genetic rules for each of the disp_mut options.

rnugraha · 2018-03-06T16:41:45Z

Ok make sense. But on the second thought, I'm also going to revisit the other solution you've mentioned on eliminating _germ. Both have their own challenging tasks.

rnugraha · 2018-03-07T19:46:22Z

@adamabeshouse Okay, so different approach. Now only using disp_germ instead of adding more suffix '_germ' to the existing mutation types. Sorting and priorities are managed by the germline comparation metrix.

adamabeshouse · 2018-03-07T21:03:00Z

@rnugraha that sounds good, thanks for doing that.

The last thing I need to ask is that you implement/adjust end to end tests having to do with the sort order. In home.spec.js, there is a section on oncoprint sorting. I'm not sure whether those tests will be affected by this change. If not, could you just add a simple test of a query that includes germline mutations, and validate the sort order using the method you can find in those tests ( checking the value of browser.execute(function() { frontendOnc.getIdOrder(); }).value)

Sjoerd-van-Hagen · 2018-03-13T11:19:48Z

@adamabeshouse could you re-review this PR and see if it is ready to be merged?

rnugraha · 2018-03-13T13:15:02Z

@adamabeshouse Unit test & e2e have been provided. Could you pls have a look?

adamabeshouse · 2018-03-13T16:58:04Z

end-to-end-tests/specs/home.spec.js

+                "patient id order correct"
+            );
+        });
+


looks good, please just add a check for sample order as well

adamabeshouse · 2018-03-13T18:16:38Z

Looks good @rnugraha could you please squash to one commit?

adamabeshouse

looks good besides comments

adamabeshouse · 2018-03-23T15:46:48Z

src/shared/components/oncoprint/DataUtils.ts

                if (oncoprintMutationType === "fusion") {
                    dispFusion = true;
                } else {
                    if (event.putativeDriver) {
                        oncoprintMutationType += "_rec";
                    }
+                    dispGermline[oncoprintMutationType] = event.mutationStatus === 'Germline';


shouldnt this be dispGermline[oncoprintMutationType] = dispGermline[oncoprintMutationType] || (event.mutationStatus === 'Germline');

please add a unit test like "fills a datum w one germline one non- " or something that could catch this

rnugraha · 2018-03-26T13:25:24Z

@adamabeshouse https://github.com/cBioPortal/cbioportal-frontend/pull/913/files#diff-831da00dd128eebcf1b83908cb0fdd1cR84 should cover the unit test for germline flag. Let me know if you think it's not sufficient.

adamabeshouse · 2018-03-26T16:57:37Z

@rnugraha thanks for making the fix. I think it's a good practice for us to make unit tests for any bugs we find, that's why I think it would be good to have a unit test where a datum is filled using two mutations - the first germline, the second not.

For testing in backend repo it should also be possible to run the tests without loading frontend over http://localhost:3000. By setting FRONTEND_TEST_DO_NOT_LOAD_EXTERNAL_FRONTEND=true one can disable this. Also include study view and patient view tests, so we can get rid of old end to end tests. Furthermore remove browser version from the filename so the tests dont pass when a different browser version is used.

rnugraha · 2018-03-27T14:31:42Z

@adamabeshouse okay, pls check again

adamabeshouse · 2018-03-27T17:45:29Z

@rnugraha could you point me to the test, I don't see it? I see a test containing two separate tests w single mutations w germline. I think it would be good to have one test where a datum is filled with two mutations, one is germline and the second is not (thats the key), and the result should be that germline is true. This would catch the bug we're talking about

"fills a datum w two germline data correctly"

rnugraha · 2018-03-27T18:36:31Z

@adamabeshouse https://github.com/cBioPortal/cbioportal-frontend/pull/913/files#diff-831da00dd128eebcf1b83908cb0fdd1cR223

adamabeshouse · 2018-03-27T18:59:03Z

@rnugraha it would be great to have a test where fillGeneticTrackDatum is called with data.length = 2, where the first element of data is Germline and the second is not

rnugraha · 2018-03-28T09:22:11Z

ah that what you mean, ok pls check now

adamabeshouse · 2018-03-28T17:41:38Z

@rnugraha thanks! can you squash your commits and then Ill merge

can you also look into the failing end-to-end tests? i think almost all of them wont have to do with these changes but maybe the last failing test about oncoprint sample order?

rnugraha · 2018-03-29T05:44:35Z

@adamabeshouse looks like all the failing tests have something to do with unable to reach certain amount of time to display something as well as the last failing test you mentioned. https://github.com/thehyve/cbioportal-frontend/blob/607915a7a919fa83f1bea77df57e382fdcb700f0/end-to-end-tests/specs/home.spec.js#L481

The e2e tests I created to test the germline seems to be fine.

adamabeshouse · 2018-03-29T18:44:05Z

@rnugraha so /index.do?cancer_study_id=acc_tcga&show_samples=true&Z_SCORE_THRESHOLD=2&RPPA_SCORE_THRESHOLD=2&data_priority=0&case_set_id=acc_tcga_cnaseq&gene_list=KRAS%2520NRAS%2520BRAF&geneset_list=+&tab_index=tab_visualize&Action=Submit&genetic_profile_ids_PROFILE_MUTATION_EXTENDED=acc_tcga_mutations&genetic_profile_ids_PROFILE_COPY_NUMBER_ALTERATION=acc_tcga_gistic&clinicallist=CANCER_TYPE,asodifjpaosidjfa,CANCER_TYPE_DETAILED,FRACTION_GENOME_ALTERED,aposdijfpoai,MUTATION_COUNT does not contain any germline mutations that may be modifying the order? if so, you just need to change the id order and commit the change to make the test pass again

rnugraha · 2018-03-30T10:47:04Z

@adamabeshouse The failed tests were not caused by my changes. The failed tests you saw are also in rc branch https://circleci.com/gh/cBioPortal/cbioportal-frontend/4458?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link.

This PR also does not affect the test with the URL you mentioned in your message. The URL you sent does not have germline mutations.

dionnezaal mentioned this pull request Jan 26, 2018

Added eighteen new mutations to study_es_0 for testing germline oncoprint display cBioPortal/cbioportal#3801

Merged

jjgao had a problem deploying to cbioportal-frontend-pr-913 January 30, 2018 18:17 Failure

adamabeshouse requested changes Feb 12, 2018

View reviewed changes

rnugraha force-pushed the germline-mutation branch from a5c2ca6 to b404537 Compare February 23, 2018 17:19

rnugraha mentioned this pull request Feb 23, 2018

Germline mutation cBioPortal/oncoprintjs#55

Closed

rnugraha force-pushed the germline-mutation branch from b404537 to 3ab38b7 Compare February 26, 2018 10:06

adamabeshouse requested changes Mar 13, 2018

View reviewed changes

rnugraha force-pushed the germline-mutation branch 4 times, most recently from 60159f1 to 98eac9e Compare March 19, 2018 16:28

rnugraha mentioned this pull request Mar 23, 2018

Display germline mutation as white stripe in oncoprint view cBioPortal/oncoprintjs#62

Merged

adamabeshouse requested changes Mar 23, 2018

View reviewed changes

rnugraha force-pushed the germline-mutation branch from 98eac9e to 6530516 Compare March 26, 2018 13:22

rnugraha force-pushed the germline-mutation branch from 05c2847 to 32b3809 Compare March 27, 2018 09:53

adamabeshouse approved these changes Mar 28, 2018

View reviewed changes

Display germline mutation as white stripe in oncoprint view

607915a

rnugraha force-pushed the germline-mutation branch from 8bc40ee to 607915a Compare March 28, 2018 18:44

adamabeshouse merged commit c1a6d90 into cBioPortal:rc Mar 30, 2018

adamabeshouse mentioned this pull request Mar 30, 2018

Update oncoprint version for germline mutations #1062

Merged

rnugraha deleted the germline-mutation branch July 25, 2018 08:57

Display germline mutation as white stripe in oncoprint view #913

Display germline mutation as white stripe in oncoprint view #913

Conversation

rnugraha commented Jan 23, 2018 • edited Loading

What? Why?

Dependency

Checks

Any screenshots or GIFs?

Before

After

In zoom-out

Notify reviewers

pieterlukasse commented Jan 25, 2018

jjgao commented Jan 26, 2018

inodb commented Jan 26, 2018

adamabeshouse commented Jan 26, 2018

jjgao commented Jan 26, 2018

Sjoerd-van-Hagen commented Jan 29, 2018

pieterlukasse commented Jan 29, 2018 • edited Loading

adamabeshouse commented Jan 29, 2018 • edited Loading

Sjoerd-van-Hagen commented Jan 29, 2018

adamabeshouse commented Jan 29, 2018

Sjoerd-van-Hagen commented Jan 29, 2018

adamabeshouse commented Jan 29, 2018

Sjoerd-van-Hagen commented Jan 29, 2018

schultzn commented Jan 29, 2018

jjgao commented Jan 29, 2018

Sjoerd-van-Hagen commented Jan 30, 2018

jjgao commented Jan 30, 2018

rnugraha commented Feb 5, 2018

rnugraha commented Feb 8, 2018 • edited Loading

pieterlukasse commented Feb 8, 2018 • edited Loading

adamabeshouse left a comment

Choose a reason for hiding this comment

adamabeshouse commented Mar 6, 2018 • edited Loading

rnugraha commented Mar 6, 2018 • edited Loading

rnugraha commented Mar 7, 2018 • edited Loading

adamabeshouse commented Mar 7, 2018

Sjoerd-van-Hagen commented Mar 13, 2018

rnugraha commented Mar 13, 2018

adamabeshouse Mar 13, 2018

Choose a reason for hiding this comment

adamabeshouse commented Mar 13, 2018

adamabeshouse left a comment

Choose a reason for hiding this comment

adamabeshouse Mar 23, 2018

Choose a reason for hiding this comment

adamabeshouse Mar 23, 2018

Choose a reason for hiding this comment

rnugraha commented Mar 26, 2018 • edited Loading

adamabeshouse commented Mar 26, 2018

rnugraha commented Mar 27, 2018

adamabeshouse commented Mar 27, 2018 • edited Loading

rnugraha commented Mar 27, 2018

adamabeshouse commented Mar 27, 2018

rnugraha commented Mar 28, 2018

adamabeshouse commented Mar 28, 2018 • edited Loading

rnugraha commented Mar 29, 2018 • edited Loading

adamabeshouse commented Mar 29, 2018 • edited Loading

rnugraha commented Mar 30, 2018 • edited Loading

rnugraha commented Jan 23, 2018 •

edited

Loading

pieterlukasse commented Jan 29, 2018 •

edited

Loading

adamabeshouse commented Jan 29, 2018 •

edited

Loading

rnugraha commented Feb 8, 2018 •

edited

Loading

pieterlukasse commented Feb 8, 2018 •

edited

Loading

adamabeshouse commented Mar 6, 2018 •

edited

Loading

rnugraha commented Mar 6, 2018 •

edited

Loading

rnugraha commented Mar 7, 2018 •

edited

Loading

rnugraha commented Mar 26, 2018 •

edited

Loading

adamabeshouse commented Mar 27, 2018 •

edited

Loading

adamabeshouse commented Mar 28, 2018 •

edited

Loading

rnugraha commented Mar 29, 2018 •

edited

Loading

adamabeshouse commented Mar 29, 2018 •

edited

Loading

rnugraha commented Mar 30, 2018 •

edited

Loading