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.

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

fixed #799 #805

Merged
merged 18 commits into from
Feb 5, 2016
Merged

fixed #799 #805

merged 18 commits into from
Feb 5, 2016

Conversation

jjgao
Copy link
Member

@jjgao jjgao commented Feb 1, 2016

This pull request is to fix issue #799.

@jjgao jjgao added this to the 1.1.0 milestone Feb 1, 2016
@jjgao
Copy link
Member Author

jjgao commented Feb 1, 2016

@pieterlukasse could you help to review?

@pieterlukasse
Copy link
Member

@jjgao : nice to see there is a class to import genes. Regarding microRNAs, maybe you should also update the class ImportMicroRNAIDs in a similar way?

@pieterlukasse pieterlukasse mentioned this pull request Feb 2, 2016
@jjgao
Copy link
Member Author

jjgao commented Feb 2, 2016

@pieterlukasse: @sheridancbio is working on better support for miRNA.. we will update ImportMicroRNAIDs if necessary.

@pieterlukasse
Copy link
Member

@jjgao : I meant the specific change where you read the symbol from column 10 instead of column 2. This same mistake (i.e. column 2) is done in ImportMicroRNAIDs.

@jjgao
Copy link
Member Author

jjgao commented Feb 2, 2016

@pieterlukasse ImportMicroRNAIDs is using another file, not the NCBI gene file.

…on.countSamplesWithProteinPosStarts()

- now construct geneId set from proteinPosStarts argument, and use for WHERE clause on table 'mutation'
- previously temporary table before join might include almost all mutations (depending on supplied geneticProfileId argument)
- changed join to an explicit INNER JOIN (from comma operator join)
@pieterlukasse
Copy link
Member

@jjgao : sorry, I realised this later. Indeed another file :) Which file/service are you using by the way?

@jjgao
Copy link
Member Author

jjgao commented Feb 3, 2016

@pieterlukasse: It's a checked in pre-cursor and mature miRNA ID mapping file. Rob is writing some script to generate that file automatically from mirbase.

@pieterlukasse
Copy link
Member

@jjgao I would like to suggest we add the following constraint to DB to correctly reflect the fact that Hugo symbols should be unique.

alter table 
 cbioportal.gene
 add
  UNIQUE KEY `UQ_HUGO_GENE_SYMBOL` (`HUGO_GENE_SYMBOL`) COMMENT 'Constraint to ensure we have unique hugo symbols';

@morungos
Copy link
Contributor

morungos commented Feb 3, 2016

Yes, the more constraints the better :-)

@pieterlukasse
Copy link
Member

@jjgao I tested importGenes.pl (as this is reported in as the "usage" string). In this commit you can find the changes I made to get it working: thehyve@f444ca0

One remark about the logging: I think it is confusing to see this message at the end:

63792 records inserted into gene_alias
59882 records inserted into gene

while in fact no records were inserted (all were already present in DB). Maybe this is another issue that needs to be logged in a new ticket?

…omutation_fix

Fix DaoMutation call countSamplesWithProteinPosStarts to help with mysql slowdown
patient view performance: hide cbioportal column in mutations table & remove mut vs cna plot
@jjgao jjgao merged commit 13e4337 into cBioPortal:rc Feb 5, 2016
@jjgao jjgao removed the in progress label Feb 5, 2016
@pieterlukasse
Copy link
Member

@jjgao : I loaded the new NCBI file using the fixed gene importer and can confirm that hugo column is now unique. In fact, I successfully applied the UQ constraint (which I mentioned before) to my DB. Could you add this UQ constraint to this PR as well?

@pieterlukasse
Copy link
Member

@jjgao : I see that my proposed fixes in previous comments were not taken up. Would you like me to send them via a separate PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants