-
Notifications
You must be signed in to change notification settings - Fork 233
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
Add OncoKB MSI-H, TMB-H annotation #3469
Conversation
<p>{this.props.geneSummary}</p> | ||
<p>{this.props.variantSummary}</p> | ||
{this.props.usingPublicOncoKbInstance ? ( | ||
<p className={mainStyles.disclaimer}> | ||
Therapeutic levels are not available in this instance of | ||
cBioPortal.{' '} | ||
<DefaultTooltip | ||
overlayStyle={{ | ||
maxWidth: 400, | ||
}} | ||
overlay={publicInstanceDisclaimerOverLay} | ||
> | ||
<i className={'fa fa-info-circle'}></i> | ||
</DefaultTooltip> | ||
</p> | ||
) : ( | ||
<> | ||
<p>{this.props.tumorTypeSummary}</p> | ||
|
||
{this.props.treatments!.length > 0 && ( | ||
<div | ||
style={{ | ||
marginTop: 10, | ||
}} | ||
> | ||
<OncoKbTreatmentTable | ||
variant={this.props.variant || ''} | ||
pmidData={this.props.pmidData!} | ||
treatments={this.props.treatments!} | ||
/> | ||
</div> | ||
)} | ||
</> | ||
)} | ||
</> |
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.
No need to review, just moved out of the tabs context below.
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.
Looks good in general, just some import issues and a few suggestions.
MSI_H, | ||
OTHER_BIOMARKER_HUGO_SYMBOL, | ||
TMB_H, | ||
} from 'react-mutation-mapper/src/component/oncokb/constants'; |
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.
Direct import from package src
directory is likely to cause a lot of issues. Let's export these constants and do this instead:
} from 'react-mutation-mapper/src/component/oncokb/constants'; | |
} from 'react-mutation-mapper'; |
We can also consider moving these constants into cbioportal-utils
if you think it is a better package for them.
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.
Not yet, let's keep it under RMM for now.
@@ -658,6 +679,87 @@ export default class PatientViewPage extends React.Component< | |||
uniqueSampleKey={sample.id} | |||
/> | |||
)} | |||
|
|||
{this.patientViewPageStore |
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.
Contents of these two spans and the render conditions are pretty similar to each other, it might be better to have a small customizable functional component for it.
MSI_H, | ||
OTHER_BIOMARKER_HUGO_SYMBOL, | ||
TMB_H, | ||
} from 'react-mutation-mapper/src/component/oncokb/constants'; |
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.
(See the previous related comment)
} from 'react-mutation-mapper/src/component/oncokb/constants'; | |
} from 'react-mutation-mapper'; |
|
||
@computed get otherBiomarkerQueries() { | ||
const queries: AnnotateMutationByProteinChangeQuery[] = []; | ||
if (_.keys(this.sampleMsiHInfo).length > 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.
The if
blocks are almost identical, may be worth combining them.
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.
Yeah, previously I don't like to add too many key value pairs for these. But since I'm gonna combine the logic in the PatientViewPage anyway, I will update here too.
src/shared/lib/StoreUtils.ts
Outdated
); | ||
} | ||
|
||
export function getNumberClinicalDataValue( |
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 we use numerical
instead of number
in the function name?
export function getNumberClinicalDataValue( | |
export function getNumericalClinicalDataValue( |
src/shared/lib/StoreUtils.ts
Outdated
} | ||
} | ||
|
||
export function getSampleNumberClinicalDataValue( |
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.
Same as above. numerical
instead of number
?
export function getSampleNumberClinicalDataValue( | |
export function getSampleNumericalClinicalDataValue( |
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.
LGTM
[sampleId: string]: { [queryType: string]: IndicatorQueryResp }; | ||
}>({ | ||
invoke: async () => { | ||
const allResult = await oncokbClient.annotateMutationsByProteinChangePostUsingPOST_1( |
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.
POST_1?
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.
Yeah, the oncokb has two sets of apis and they sharing the same name. The swagger generates as POST_1.
}; | ||
}); | ||
const map = _.chain(updatedResult) | ||
.groupBy('sampleId') |
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.
string shortcut not typesafe
}); | ||
const map = _.chain(updatedResult) | ||
.groupBy('sampleId') | ||
.mapValues(group => _.keyBy(group, 'type')) |
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 not string shortcuts
export function getNumericalClinicalDataValue( | ||
clinicalData: ClinicalData | ||
): number | undefined { | ||
if (Number.isNaN(clinicalData.value)) { |
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.
Just checking but this assumes that clinicalData.value is numerical (if in string form). Number.isNaN("blah") is false so you could get Number("blah") which would be NaN.
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.
Correct.
Do not show subtitle when hugoSymbol is Other Biomarker Signed-off-by: Hongxin Zhang <hongxin@cbio.mskcc.org>
The failing tests are irrelevant to this pr. |
The following annotations will be added when sample has TMB, MSI scores.
The thresholds for both score are >=10.
Patient View Header
MSI-H Annotation
TMB-H Annotation
Fix cBioPortal/cbioportal#6862