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

Big bed file with more than 256 contigs is handled properly. #520

Merged

Conversation

piotr-gawron
Copy link
Contributor

Current implementation of BigBed file doesn't handle properly files with more than 256 contigs. This is due to the assumption that contigs are a flat structure (

// Just assume it's a flat "tree" for now.
)

This PR resolve the issue by parsing contig tree structure recursively if necessary.

I hope it will meet your coding standards. I couldn't find a way to enforce jbinary to parse dynamic recursive structures (but it might be doable). Therefore, when the situation arises I manually call jbinary to parse nodes recursively.

@piotr-gawron
Copy link
Contributor Author

Build failed due to http-party/http-server#521 and jfhbrook/node-ecstatic#255

@thornjad
Copy link

thornjad commented May 2, 2019

@akmorrow13
Copy link
Collaborator

akmorrow13 commented May 2, 2019

Thanks for the PR @piotr-gawron! Looks like you all are working on backward compatibility right now with the http-server. Once that gets in I can re-trigger the tests and take another look at this.

@piotr-gawron
Copy link
Contributor Author

piotr-gawron commented May 3, 2019

Dear @akmorrow13,
I upgraded problematic dependency and re-triggered CI.

Now travis is failing, but I'm not sure I understand why - is the flow check out of date?.
The unit test is written properly - for simplicity you can return a promise instead of calling done callback (https://mochajs.org/#working-with-promises). Moreover, _.keys is a valid function in underscore (otherwise the test would fail).

src/test/data/BigBed-test.js Outdated Show resolved Hide resolved
src/test/data/BigBed-test.js Show resolved Hide resolved
src/test/components-test.js Show resolved Hide resolved
@akmorrow13
Copy link
Collaborator

Thanks for the update @piotr-gawron ! I added some comments in the review for how you can fix the flow errors.

@piotr-gawron
Copy link
Contributor Author

@akmorrow13, I fixed flow issues. Let me know if you have more comments.

@coveralls
Copy link

coveralls commented May 3, 2019

Coverage Status

Coverage increased (+0.04%) to 91.499% when pulling a1893b4 on piotr-gawron:big-bed-file-with-257-contigs into e99848d on hammerlab:master.

}

// Generate the reverse map from contig ID --> contig name.
function reverseContigMap(contigMap: {[key:string]: number}): Array<string> {
function reverseContigMap(contigMap: { [key: string]: number }): Array<string> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like your editor modifier a lot of the spacing/indentation in this file. Would you be able to revert the lines you did not intend to modify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for that.
I reverted the indentation

@akmorrow13 akmorrow13 merged commit a9165cd into hammerlab:master May 6, 2019
@akmorrow13
Copy link
Collaborator

Thanks @piotr-gawron!

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.

None yet

4 participants