Skip to content
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.

Sign up for GitHub

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

Jump to bottom

use grch38 genome nexus for grch38 studies #3147

Merged
merged 1 commit into from May 5, 2020
Merged

use grch38 genome nexus for grch38 studies #3147

merged 1 commit into from May 5, 2020

Conversation

leexgh
Copy link
Member

@leexgh leexgh commented Apr 14, 2020 •
edited by inodb

Fix: cBioPortal/cbioportal#7368

If the study has mutations based on grch38, use https://grch38.genomenexus.org

For now we won't support studies having both grch37 and grch38 mutation data. It will always use the grch37 genome nexus instance in that case.

@leexgh leexgh marked this pull request as draft April 14, 2020 16:16
@leexgh leexgh marked this pull request as ready for review April 21, 2020 22:10
@leexgh leexgh force-pushed the portal-hg38 branch 2 times, most recently from c9909c1 to 80ca7aa Compare April 23, 2020 20:08
alisman
alisman reviewed Apr 24, 2020
View reviewed changes
src/pages/patientView/clinicalInformation/PatientViewPageStore.ts Outdated
@@ -1623,4 +1648,37 @@ export class PatientViewPageStore {
},
[]
);

// if we use grch37(default), grch38GenomeNexusUrl will be undefined
@computed get grch38GenomeNexusUrl() {
Copy link
Collaborator

@alisman alisman Apr 24, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value is serving as the "source of truth" of the current build. I don't think the name is abstract enough. It maybe that genome nexus is the only feature of the portal which is genome build dependent, but the fact that it's a property of studies suggests is something more general.
In other words, I'm a developer, i want to know what the current build is. I wouldn't think to check whether grch38GenomeNexusUrl is undefined. I would be looking for "currentReferenceGenemeBuild" or some such abstract property. It can contain the same logic here (based on study result).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

onursumer
onursumer reviewed Apr 24, 2020
View reviewed changes
src/pages/resultsView/ResultsViewPageStore.ts Outdated
@@ -3187,6 +3206,45 @@ export class ResultsViewPageStore {
[]
);

// if we use grch37(default), grch38GenomeNexusUrl will be undefined
@computed get grch38GenomeNexusUrl() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is maybe out of the scope of this PR but we have a lot of "almost identical" code segments in our stores. It would be great if we can somehow unify these similar methods.

For this method though, it might be possible to move the logic into a utility function.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@onursumer where would be a good place to move to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have StoreUtils.ts. We can maybe add a function there if we don't have a more specific utility file.

onursumer
onursumer requested changes Apr 27, 2020
View reviewed changes
Copy link
Member

@onursumer onursumer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in the function name

src/shared/lib/StoreUtils.ts Outdated Show resolved Hide resolved
@alisman
Copy link
Collaborator

alisman commented Apr 28, 2020

@leexgh so, MutationTable component should have a property, buildWhateverLink:(blah)=>string;

This function will be called inside the column renderer for our column. We define the function ... maybe in ReusltsViewMutationMapper which has access to the store.

@leexgh leexgh force-pushed the portal-hg38 branch 2 times, most recently from e707ff7 to e3f016c Compare May 4, 2020 15:42
@leexgh leexgh requested a review from onursumer May 4, 2020 16:57
@inodb inodb self-assigned this May 4, 2020
inodb
inodb reviewed May 5, 2020
View reviewed changes
src/shared/lib/StoreUtils.ts Outdated Show resolved Hide resolved
inodb
inodb approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@inodb inodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after @onursumer comments are addressed.

I made one minor comment about renaming a function

@leexgh leexgh requested a review from onursumer May 5, 2020 16:44
@alisman alisman merged commit 24d19c0 into cBioPortal:master May 5, 2020
@inodb inodb added the cl-external-api External API section of changelog. Changes to handle an external API (i.e. not cBioPortal API) label May 7, 2020
@inodb inodb changed the title support grch38 in portal use grch38.genomenexus.org for grch38 studies May 7, 2020
@inodb inodb changed the title use grch38.genomenexus.org for grch38 studies use grch38 genome nexus for grch38 studies May 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisman alisman alisman approved these changes

@inodb inodb inodb approved these changes

@onursumer onursumer onursumer approved these changes

Assignees

@inodb inodb

Labels
cl-external-api External API section of changelog. Changes to handle an external API (i.e. not cBioPortal API)
Projects
None yet
Milestone
No milestone
4 participants
@leexgh @alisman @inodb @onursumer