-
Notifications
You must be signed in to change notification settings - Fork 588
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
Db migration: remove gene length #6312
Conversation
@sheridancbio i forgot what the rules are regarding schema versioning. Where can I find that again? |
411c9bc
to
04db2e6
Compare
a327ad8
to
38228fb
Compare
@@ -1,259 +0,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.
don't think this is used anymore either, all pfam data is in genome nexus
@@ -362,15 +362,6 @@ | |||
<url-pattern>/omaRedirect.do</url-pattern> | |||
</servlet-mapping> | |||
|
|||
<servlet> |
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.
don't think this is used anymore either, all pfam data is in genome nexus
This information is stored in Genome Nexus. We don't need it in cBioPortal anymore. Fix cBioPortal#6308
38228fb
to
dd10a15
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.
thanks @inodb . Happy to see some cleanup taking place! Looks good to me 👍
Only one small request: please also update the documentation here https://github.com/cBioPortal/cbioportal/blob/master/docs/Updating-gene-and-gene_alias-tables.md#mysql-steps (step 5).
Thanks @pieterlukasse ! Good catch! Updated That reminds me we might have to do a mouse version of genome nexus: genome-nexus/genome-nexus-importer#22 I don't think mouse support for cBioPortal will work currently: |
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.
thanks @inodb! Good point regarding mouse support. Thanks for logging the issue 👍
Removing gene length is a pretty substantial change
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.
code looks good, just 2 minor requests:
- remove var declarations for
TEST_LENGTH*
in test classes (see comments) - update usage statement in documentation
web/src/test/java/org/cbioportal/web/DiscreteCopyNumberControllerTest.java
Show resolved
Hide resolved
@@ -132,7 +130,6 @@ public void getGene() throws Exception { | |||
gene.setHugoGeneSymbol(HUGO_GENE_SYMBOL_1); | |||
gene.setType(TYPE_1); | |||
gene.setCytoband(CYTOBAND_1); | |||
gene.setLength(LENGTH_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.
same comment here about removing the LENGTH*
vars
@@ -283,7 +283,6 @@ public void getMutationsInMolecularProfileBySampleListIdDetailedProjection() thr | |||
.andExpect(MockMvcResultMatchers.jsonPath("$[0].gene.hugoGeneSymbol").value(TEST_HUGO_GENE_SYMBOL_1)) | |||
.andExpect(MockMvcResultMatchers.jsonPath("$[0].gene.type").value(TEST_TYPE_1)) | |||
.andExpect(MockMvcResultMatchers.jsonPath("$[0].gene.cytoband").value(TEST_CYTOBAND_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.
same comment here about removing the TEST_LENGTH*
vars
@ao508 Thanks for catching those ! Updated the PR |
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.
approving changes
@inodb Gene length appears to be used in the lollipop plot and oncoprint components so I do not think this can be removed from GN. private get proteinLength(): number {
return (this.props.store.allTranscripts.result &&
this.props.store.activeTranscript &&
this.props.store.transcriptsByTranscriptId[this.props.store.activeTranscript] &&
this.props.store.transcriptsByTranscriptId[this.props.store.activeTranscript].proteinLength) ||
Math.round(this.props.store.gene.length / 3);
} function formatGeneticTrackSublabel(oqlFilter: UnflattenedOQLLineFilterOutput<object>): string {
if (isMergedTrackFilter(oqlFilter)) {
return ""; // no oql sublabel for merged tracks - too cluttered
} else {
return oqlFilter.oql_line.substring(oqlFilter.gene.length).replace(";",""); // get rid of gene in beginning of OQL line, and semicolon at end
}
} |
I misunderstood the idea of the PR. It was remapping of gene.length from cBP-db to GN. Never mind my comment. |
Hi @inodb ... here were old rules we adopted for db schema version number assignment. I'm not sure what our current practice is. https://github.com/cBioPortal/cbioportal/wiki/EP-Review-Changes-to-Database-Schema |
This information is stored in Genome Nexus. We don't need it in
cBioPortal anymore.
Fix #6308