Skip to content
This repository has been archived by the owner on Jan 24, 2018. It is now read-only.

Bigwig #1573

Merged
merged 14 commits into from
Feb 21, 2017
Merged

Bigwig #1573

merged 14 commits into from
Feb 21, 2017

Conversation

ejacox
Copy link
Collaborator

@ejacox ejacox commented Feb 15, 2017

Adds server code to implement the continuous sequence annotation service as part of ga4gh/ga4gh-schemas#769.

@david4096
Copy link
Member

david4096 commented Feb 16, 2017

Really stellar code! Just took a look and it follows the existing patterns clearly.

Noting that the only failing test is test_constraints, which is by design.

requirements.txt Outdated
@@ -53,6 +53,8 @@ requests==2.7.0
oic==0.7.6
pyOpenSSL==0.15.1
lxml==3.4.4
pyBigWig==0.3.2
numpy==1.11.3
Copy link
Member

Choose a reason for hiding this comment

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

Does it get used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not anymore. Removed.

The output is in wiggle format, which is processed by the WiggleReader
class.

There could be memory issues if the returned results are large.
Copy link
Member

Choose a reason for hiding this comment

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

This is a very clever usage of the existing tools to support the BigWig format using the reader you wrote! You note the issues of running a subprocess. It seems like you opted to use the existing python tools, which I think is a very sane choice. Would you like to leave this in for future feature support? Is it used?

@david4096
Copy link
Member

david4096 commented Feb 16, 2017

Would you please add an entry to the templates/index.html for the continuous sets? Something like https://github.com/ga4gh/server/blob/master/ga4gh/server/frontend.py#L184

Although it is a problem in the reminder of the API, we might choose to not adopt the assumption that a reference set must be set on the continuous set. We'll do that in a later PR (#1528)

@david4096
Copy link
Member

I had to install the libcurl4-gnutls-dev to successfully install PyBigWig. I wasn't able to replicate this issue uninstalling the package though. PyBigWig seems to want to have curl-config around for accessing remote BigWig files.

@david4096 david4096 merged commit 81078fe into ga4gh:master Feb 21, 2017
@david4096
Copy link
Member

david4096 commented Feb 21, 2017

Congratulations @ejacox this is a sizable addition to the protocol and does a good job at making some of Jim Kent's (and other's) work available to a wider community!

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

Successfully merging this pull request may close these issues.

2 participants