-
Notifications
You must be signed in to change notification settings - Fork 91
Conversation
Really stellar code! Just took a look and it follows the existing patterns clearly. Noting that the only failing test is |
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 |
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.
Does it get used?
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.
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. |
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 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?
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
|
I had to install the |
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! |
Adds server code to implement the continuous sequence annotation service as part of ga4gh/ga4gh-schemas#769.