-
Notifications
You must be signed in to change notification settings - Fork 175
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
[Raisinbread] Added CNV/CPG records for candidates with SNP data #6900
[Raisinbread] Added CNV/CPG records for candidates with SNP data #6900
Conversation
@@ -0,0 +1,25 @@ | |||
--Add CNV data for some candidates |
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.
files in this directory get run by real projects with real data. this patch is adding mock data useful for testing.
This addition while very very helpful should not be in the patch directory. it's sufficient to make the changes to the RB files only.
if it makes it easier to understand:
- anything that should be ran on all projects should also be ran on RB
- however patches created FOR RB should never be ran on projects
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.
Okay this makes sense. I'll remove the patch and leave the changes in the Raisinbread files.
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.
SQL changes look good
Since the issue was created by @HenriRabalais, maybe you can give it a try ?
Basically just make sure the module loads with this data and links are clickable/accessible where it applies. you can also try to evaluate the "quality" of the data but thats going the extra mile. obviously some data is better than none so this PR should be sufficient on its own
3976d11
to
58d2d23
Compare
alex could you post a screenshot of what it looks like now? |
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 fine by me. 2 comments:
- the candidate IDs you used : do they have imaging data too? if there are other candidates with genomic records (e.g. snps) && imaging data - please use their id's instead.
- could you add Rida's "how to contribute a change to raisinbread" instructions to the Loris cheat sheet ? (I'm on the fence but the internal guide seems like the place for this). this info should be available to the team.
@christine The candidate's I used do have SNP data but I will check if they also have imaging data. I will add that content to the internal doc :) |
I can't tell from the thread.. is this ready to be merged or does it need a doc updated? |
@driusan It does not need a doc update. After Christine's comment, I checked that the candidate's have SNP data and they do, so I think this is all set. |
Added CNV and CPG records for candidates that already have SNP data. This is necessary so that the
CNV
andMethylation
tabs in the Genomic Browser module can be tested.Note: I can add more data if more is needed or requested.
Testing instructions
With a fresh install of RB or after running the patch:
Genomic Browser
.CNV
andMethylation
tabs now display and can filter data.Links to related issues