-
Notifications
You must be signed in to change notification settings - Fork 63
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
Update BED parsing #479
base: master
Are you sure you want to change the base?
Update BED parsing #479
Conversation
Addresses #477 |
@akmorrow13 Hmm... how do I get the Travis results to show up somewhere locally before I push? I ran the tests, but it showed the same test failures locally as it did with the master branch... |
@denisemauldin travis runs remotely so you can not run it locally. However, you can run all of the commands that travis runs, listed in the .travis.yml file. It looks like |
position, | ||
id: x[0], // e.g. ENST00000359597 | ||
strand: x[2], // either + or - | ||
codingRegion: new Interval(Number(x[3]), Number(x[4])), |
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 will still fail if you only have position information. x[0]-x[10] may be missing as well. I remember you mentioning that your bed files only had contig, start and end. Maybe I misunderstood.
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.
Yeah, I think I changed my definition of what I wanted to support after I wrote this bit and didn't go back to update it.
@@ -57,7 +57,7 @@ function drawGeneName(ctx: CanvasRenderingContext2D, | |||
textIntervals: Interval[]) { | |||
var p = gene.position, | |||
centerX = 0.5 * (clampedScale(1 + p.start()) + clampedScale(1 + p.stop())); | |||
var name = gene.name || gene.id; | |||
var name = gene.name || gene.id || gene.position.toString(); |
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.
We could actually put this in a separate PR, this seems useful.
gene.exons.forEach(range => { | ||
drawArrow(ctx, clampedScale, range, geneLineY + 0.5, gene.strand); | ||
}); | ||
if (gene.strand && gene.exons && gene.codingRegion) { |
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 don't think we want to modify the GeneTrack code. If strand and exons and codingRegion are missing, FeatureTrack would be a better fit for the data.
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 agree that a FeatureTrack is a better fit for the data, but I don't know which data is expected for each visualization type. I don't know when I'm supposed to use a FeatureTrack vs a CoverageTrack vs a GenomeTrack vs a GeneTrack when I'm a naive user with a BAM or BED file in hand.
I try to utilize defensive programming for user naivety and make sure something runs and doesn't throw errors if the user attempts to do something not intended by the code. Wrapping this set of code in an if statement makes it so that the GeneTrack loads even when provided a simple BED file with 3 columns.
Without these changes, GeneTrack throws:
bedtools.js:21 Uncaught TypeError: Cannot read property 'map' of undefined
at Object.splitCodingExons (bedtools.js:21)
at GeneTrack.js:147
at Array.forEach (<anonymous>)
at GeneTrack.updateVisualization (GeneTrack.js:136)
at GeneTrack.componentDidUpdate (GeneTrack.js:104)
at CallbackQueue.notifyAll (CallbackQueue.js:65)
at ReactReconcileTransaction.close (ReactReconcileTransaction.js:81)
at ReactReconcileTransaction.closeAll (Transaction.js:202)
at ReactReconcileTransaction.perform (Transaction.js:149)
at ReactUpdatesFlushTransaction.perform (Transaction.js:136)
Cool. I've never done flow before. I'll check it out 😄 |
This is my version that I was working on before I saw yours @akmorrow13 ! Would you mind reviewing this anyway? Do you think it would be useful for me to submit the tests as a separate PR?
This change is