-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature to visualize mutational counts for mutational signatures #4540
Feature to visualize mutational counts for mutational signatures #4540
Conversation
b7845f1
to
3da7e1d
Compare
3da7e1d
to
61719d6
Compare
src/pages/patientView/clinicalInformation/ClinicalInformationMutationalSignatureTable.tsx
Show resolved
Hide resolved
src/pages/patientView/clinicalInformation/ClinicalInformationMutationalSignatureTable.tsx
Outdated
Show resolved
Hide resolved
src/pages/patientView/clinicalInformation/ClinicalInformationMutationalSignatureTable.tsx
Show resolved
Hide resolved
src/pages/patientView/clinicalInformation/ClinicalInformationMutationalSignatureTable.tsx
Show resolved
Hide resolved
src/pages/patientView/clinicalInformation/ClinicalInformationMutationalSignatureTable.tsx
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignaturesContainer.tsx
Outdated
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignaturesContainer.tsx
Outdated
Show resolved
Hide resolved
signature: string; | ||
show: boolean; | ||
onHide: () => void; | ||
parentCallback: (childData: string, visibility: boolean) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename the function and param names as discussed in previous comments.
src/pages/patientView/mutationalSignatures/SignatureTextBox.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change matrix
key to counts
(we're loading mutational count matrices with the stable_id suffix _counts_{ID}
59893d2
to
e322ec6
Compare
@@ -16,7 +16,7 @@ export enum MutationalSignaturesVersion { | |||
export enum MutationalSignatureStableIdKeyWord { | |||
MutationalSignatureContributionKeyWord = 'contribution', | |||
MutationalSignatureConfidenceKeyWord = 'pvalue', | |||
MutationalSignatureCountKeyWord = 'counts', | |||
MutationalSignatureCountKeyWord = 'matrix', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? It looks like the keyword reverted back to matrix
e1f1d4b
to
23bf5d4
Compare
1ab4589
to
dbe61fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TJMKuijpers @MatthijsPon Thanks for making this enhancement, It looks good to me. I have some minor comments. @onursumer Might have more comments about this pr.
const nameSig: string = | ||
'MUTATION_TYPE' in metaData.genericEntityMetaProperties | ||
? metaData.genericEntityMetaProperties['MUTATION_TYPE'] | ||
: ''; | ||
const classSig: string = | ||
'MUTATION_CLASS' in metaData.genericEntityMetaProperties | ||
? metaData.genericEntityMetaProperties['MUTATION_CLASS'] | ||
: ''; | ||
const mutNameSig: string = | ||
'NAME' in metaData.genericEntityMetaProperties | ||
? metaData.genericEntityMetaProperties['NAME'] | ||
: ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a util function that can be used to replace this kind of logic (get property from GenericAssayMeta): https://github.com/cBioPortal/cbioportal-frontend/blob/master/src/shared/lib/GenericAssayUtils/GenericAssayCommonUtils.ts#LL277C17-L277C53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that function, thank you! I have replaced the logic with the following (for each property):
const name:string=getGenericAssayMetaPropertyOrDefault(metaData,'NAME','')
mutationalSignatureChartData.uniqueSampleKey = | ||
count.uniqueSampleKey; | ||
mutationalSignatureChartData.version = | ||
_.last(count.molecularProfileId.split('_')) || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we extract logic here to make it a util function, e.g. getMutationalSignatureVersionFromProfileId
? I believe this function will be shared across lots of files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted this logical to the proposed function getMutationalSignatureVersionFromProfileId
export function getMutationalSignaturesVersionFromProfileId(inputProfileId:string):string{
return _.last(inputProfileId.split('_')) || '';
}
return Promise.resolve(genericAssayRawData); | ||
} else { | ||
return Promise.resolve(genericAssayRawData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
seems useless. If not valid, should we return an empty result or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the function validateMutationalSignatureRawData
and the if/else statement are not necessary anymore. We have added the option to upload mutational signature data without p-values. This is needed for some tools that do provide contribution and count data but no p-values.
So now we can return the genericAssayRawData
if mutationalSignatureMolecularProfileIds.length > 0
or []
@observer | ||
class legendLabels extends React.Component {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed it
for (const count of inputData) { | ||
let mutationalSignatureChartData: IMutationalCounts = {} as IMutationalCounts; | ||
mutationalSignatureChartData.patientId = count.patientId; | ||
mutationalSignatureChartData.sampleId = count.sampleId; | ||
mutationalSignatureChartData.studyId = count.studyId; | ||
mutationalSignatureChartData.uniquePatientKey = | ||
count.uniquePatientKey; | ||
mutationalSignatureChartData.uniqueSampleKey = | ||
count.uniqueSampleKey; | ||
mutationalSignatureChartData.version = getMutationalSignaturesVersionFromProfileId( | ||
count.molecularProfileId | ||
); | ||
mutationalSignatureChartData.value = parseFloat(count.value); | ||
mutationalSignatureChartData.mutationalSignatureLabel = | ||
signatureLabelMap | ||
.filter(obj => obj.stableId === count.stableId) | ||
.map(obj => obj.name)[0] || ''; | ||
result.push(mutationalSignatureChartData); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use map
here, something like this:
for (const count of inputData) { | |
let mutationalSignatureChartData: IMutationalCounts = {} as IMutationalCounts; | |
mutationalSignatureChartData.patientId = count.patientId; | |
mutationalSignatureChartData.sampleId = count.sampleId; | |
mutationalSignatureChartData.studyId = count.studyId; | |
mutationalSignatureChartData.uniquePatientKey = | |
count.uniquePatientKey; | |
mutationalSignatureChartData.uniqueSampleKey = | |
count.uniqueSampleKey; | |
mutationalSignatureChartData.version = getMutationalSignaturesVersionFromProfileId( | |
count.molecularProfileId | |
); | |
mutationalSignatureChartData.value = parseFloat(count.value); | |
mutationalSignatureChartData.mutationalSignatureLabel = | |
signatureLabelMap | |
.filter(obj => obj.stableId === count.stableId) | |
.map(obj => obj.name)[0] || ''; | |
result.push(mutationalSignatureChartData); | |
} | |
result = inputData.map(count => ({ | |
patientId: count.patientId, | |
sampleId: count.sampleId, | |
studyId: count.studyId, | |
uniquePatientKey: count.uniquePatientKey, | |
uniqueSampleKey: uniqueSampleKey, | |
version: getMutationalSignaturesVersionFromProfileId( | |
count.molecularProfileId | |
), | |
value: parseFloat(count.value), | |
mutationalSignatureLabel: signatureLabelMap | |
.filter(obj => obj.stableId === count.stableId) | |
.map(obj => obj.name)[0] || '', | |
}) | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the map function and created the type MutationalSignatureCount
so that count
is not of type any
@@ -220,6 +229,18 @@ export type PathologyReportPDF = { | |||
url: string; | |||
}; | |||
|
|||
export type confidenceDataMapType = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types should be named in title case, not camel case. Also, we probably don't need the Type
suffix in the name.
export type confidenceDataMapType = { | |
export type ConfidenceDataMap = { |
Maybe not export this type for now, since it seems like we only use it in this store. If we really want to import this type in other places, maybe better to export this in a utility class or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the export
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChart.spec.tsx
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChart.tsx
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChart.tsx
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChartUtils.ts
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChartUtils.ts
Outdated
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChartUtils.ts
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/MutationalSignatureBarChartUtils.ts
Outdated
Show resolved
Hide resolved
src/pages/patientView/mutationalSignatures/SignatureTextBox.tsx
Outdated
Show resolved
Hide resolved
9c6df57
to
320236e
Compare
<div | ||
style={{ | ||
maxWidth: 250, | ||
fontFamily: 'Helvetica Neue, Helvetica, Arial, sans-serif', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not be changing font or color. ideally not size either. i think there is a tooltip class that applies standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SignatureTextBox
is replaced by the standard React ToolTip element.
overlayClassName="oncokb-tooltip" | ||
overlay={this.tooltipInfo} | ||
> | ||
<a href="#"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the SignatureTextBox and made use of the React Tooltip
// name format: ENTITY_NAME (CATEGORY) | ||
// we can get category between '(' and ')' | ||
return name | ||
? name.substring(name.lastIndexOf('(') + 1, name.lastIndexOf(')')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use RexExp.match for this kind of thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replaced with:
const regExpName = new RegExp('\\(.*?\\)')
return name
? regExpName.exec(name)![0]:'No category';
} | ||
|
||
@observer | ||
export default class MutationalSignaturesContainer extends React.Component< | ||
IMutationalSignaturesContainerProps, | ||
{} | ||
> { | ||
@observable signatureProfile: string; | ||
@observable.ref private plotSvg: SVGElement | null = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is strange, to make a dom element observable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed the PlotsTab.tsx
implementation but for the mutational signature plot it should indeed not be an observable (PlotsTab.tsx
has multiple types of plots). So I changed it to: private plotSvg: SVGElement | null = null;
@observable.ref private plotSvg: SVGElement | null = null; | ||
@observable signatureURL: string; | ||
@observable signatureDescription: string; | ||
@observable signatureInformationToolTipVisible: boolean = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isSignatureInformationToolTipVisible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
this.signatureProfile = childData; | ||
this.signatureInformationToolTipVisible = visibility; | ||
this.signatureURL = this.props.data[this.props.version].filter(obj => { | ||
if (childData === obj.meta.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter method should always return boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced wtih return childData === obj.meta.name
if (childData === obj.meta.name) { | ||
return obj; | ||
} | ||
})[0].meta.url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should handle case where it's not found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the ternary statement that checks whether the output of the filter has a length >0, if yes it will return the meta.description
, otherwise No description available
.
For the URL, it will return an empty string and in the tooltip we display 'No link to external website available'
.
} | ||
})[0].meta.url; | ||
this.signatureDescription = this.props.data[this.props.version].filter( | ||
obj => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again filter callback should return boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced wtih return childData === obj.meta.name
); | ||
} | ||
|
||
@action.bound changeSignature(name: string): void { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setSignature or use get/set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changeSignature
is not needed anymore with the new Tooltip in the mutational table
this.signatureInformationToolTipVisible = false; | ||
} | ||
|
||
@action.bound |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't really need to be action because it isn't directly mutating state. it's the callback (props.onSampleChange) which probably needs to be action.
|
||
@autobind | ||
private getSvg() { | ||
if (document.getElementById('mutationalBarChart')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look up React reference https://legacy.reactjs.org/docs/refs-and-the-dom.html to handle dom reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a reference for the dom element and removed the id
constructor(props: IMutationalSignaturesContainerProps) { | ||
super(props); | ||
makeObservable(this); | ||
} | ||
|
||
@observable _selectedSignature: string = this.selectURLSignature; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is kind of odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned this part of the code, _selectedSignature
is not needed anymore.
fe5921d
to
72ad823
Compare
ab8733c
to
81009ba
Compare
f9418a0
to
dfeadd3
Compare
777b845
to
4e54771
Compare
Updated test study for COSMIC version SBS, DBS, and ID (test mutational count values)
a4368f9
to
585f13b
Compare
@observer | ||
export default class ClinicalInformationMutationalSignatureTable extends React.Component< | ||
IClinicalInformationMutationalSignatureTableProps, | ||
{} | ||
> { | ||
@observable selectedSignature = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty string is not a good default value. lets fix after merge
The challenge
Mutational signatures are typically visualized as a histogram, where the X-axis represents the recorded mutation classes (e.g. A[C>A]A for single-base substitution signatures). Similarly, double-base substitutions (DBS) and indel (ID) signatures can be visualized. What is currently missing, is a way to visualize the signature histograms of a patient and how that relates to the reference signatures that were used in the fitting process.
The cBioPortal frontend will be extended to recognize mutational signature count data and in the Patient View, the Mutational Signature tab is extended to plot histograms of the counts for the selected signature in the table.. This is the front-end implementation of RFC64.
Implementation
Describe changes proposed in this pull request:
PatientViewPage
: to handle the sample change to show different count profilesPatientViewPageTabs
:functionality to handle patients with multiple samplesClinicalInformationMutationalSigantureTable
: Update the table to have a 'clickable' entry which launches the modal.PatientViewPageStore
has the following changes:-
fetchAllMutationalSignatureCountMetaData
to extract the count data from the generic assay data.-
selectedSampleUniqueKeyMutationalSignatureData
to obtain the counts for the selected sample.-
samplesWithCountDataAvailable
to obtain all samples with count dataMutationalSignatureBarChart
: to create the bar chart with mutational count dataMutationalSignaturesContainer
: Updated to show the mutational count data and handle theMutationalSignatureTextBox
: on hoover over a signature, this shows the mutational signature information and a link to an external database (in this case COSMIC).Download button to save the mutational bar chart as svg or png.
Note: this PR is linked to cBioPortal/datahub-study-curation-tools#48 (backend implementation) and cBioPortal/datahub#1784 (containing some studies with extracted mutational signatures)
Screenshot test
new tests:
Updated tests
Updated study for local test:
Important:Counts are randomly generated and do not reflect real mutational signatures!
Images showing the new feature
View for mutational count. Patient with multiple samples will have an additional drop down menu to switch between samples.
View for the mutational counts for DBS:
View for the mutational counts for ID:
Image when SBS counts are displayed as percentage:
To gain a better understanding of a signature, the user can click on a signature in the mutational signature table to launch a modal. This modal shows the basic signature information and the link to the cosmic website.