-
Notifications
You must be signed in to change notification settings - Fork 315
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
Fix incorrect oncokb data and cancer type label used in mutations table #4758
Fix incorrect oncokb data and cancer type label used in mutations table #4758
Conversation
if (this.getSameTumorTypeAttribute(data) === 'CANCER_TYPE_DETAILED') { | ||
return this.resolveTumorType; | ||
} | ||
if (this.getSameTumorTypeAttribute(data) === 'CANCER_TYPE') { | ||
return this.resolveCancerType; | ||
} |
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 resolveTumorType
vs resolveCancerType
naming feels a little weird, tho i guess it was already there. Not super clear what the difference between those is
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.
yea, I wasn't sure what to name it but resolveTumorType
already exists and typically gets the Cancer Type Detailed
from a mutation while the newly added resolveCancerType
will retrieve the Cancer Type
. any ideas on name changes? throughout the codebase, the term Tumor Type
is used but mostly just for Cancer Type Detailed
and I didn't want to change that.
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 see - yeah as long as it is consistent that's fine by me. If we were to change it throughout the code I'd prolly be explicit and just say resolveCancerType vs resolveCancerTypeDetailed
src/shared/lib/StoreUtils.ts
Outdated
// // first priority is CANCER_TYPE_DETAILED in clinical data | ||
// _.each(clinicalDataForSamples.result, (clinicalData: ClinicalData) => { | ||
// if (clinicalData.clinicalAttributeId === 'CANCER_TYPE_DETAILED') { | ||
// map[clinicalData.uniqueSampleKey] = 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.
maybe good to remove commented out code?
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 great! Functionality seems to work from product perspective. Made a few minor comments on the code
@gblaih did you respond to Ino's comments? |
…le (#4758) * fix incorrect cancer type and oncokb levels in mutations table --------- Co-authored-by: Bryan Lai <laib1@mskcc.org>
Fixes cBioPortal/cbioportal#10421
Currently, when
CANCER_TYPE_DETAILED
is the same for all mutations of a row in the table, it uses this to fetch the oncoKb data; otherwise, it uses "Cancer of Unknown Primary".This fixes it so that if there are multiple
CANCER_TYPE_DETAILED
for mutations of a row, it checks ifCANCER_TYPE
is the same and if so, uses that to fetch the oncoKb data. If there are multipleCANCER_TYPE
as well, then it uses "Cancer of Unknown Primary".