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

Update BED parsing #479

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

denisemauldin
Copy link

@denisemauldin denisemauldin commented Jun 26, 2018

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 Reviewable

@denisemauldin
Copy link
Author

Addresses #477

@denisemauldin
Copy link
Author

@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...

@akmorrow13
Copy link
Collaborator

@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 flow check is failing for this PR.

position,
id: x[0], // e.g. ENST00000359597
strand: x[2], // either + or -
codingRegion: new Interval(Number(x[3]), Number(x[4])),
Copy link
Collaborator

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.

Copy link
Author

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();
Copy link
Collaborator

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) {
Copy link
Collaborator

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.

Copy link
Author

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)

@denisemauldin
Copy link
Author

Cool. I've never done flow before. I'll check it out 😄

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.

2 participants