search

lawremi / rtracklayer

R interface to genome annotation files and the UCSC genome browser
Other
29 stars 17 forks source link

fix bug in reading chain files #1

Closed mlist closed 7 years ago

mlist commented 7 years ago

In commit a3c430 a small bug was introduced in an attempt to close open connections provided to the method. this is fixed here.

lawremi commented 7 years ago

Thanks for the report and suggested fix. Are you sure the code inside the if() block will work with this change?

mlist commented 7 years ago

I tested it. The fix restores the previous functionality that chain files can be loaded when a path to the chain file is provided as string. As for the actual test that is done here, I don't think it is actually ever tested if con is a connection. If I use a file connection instead of a path this is already caught by

stop("'filename'` must be a single string, specifying a path") in the class ChainFile.

lawremi commented 7 years ago

The connection() function can return a gzip connection for compressed files, or a url connection for non-file URLs. The ChainFile() constructor is not catching that right now. I guess it might be best to perform this check in the constructor, not at import time, although currently the file objects do not assume that a resource actually exists at the path. Only import() has to assume that.

So just to clarify, did you test that the code in the if() block works? I'm not sure it will with this change.

mlist commented 7 years ago

Yes I did test it with the DeepBlueR package:

library(DeepBlueR)
data_id = deepblue_select_experiments(
experiment_name="E002-H3K9ac.narrowPeak.bed", chromosome="chr1")
request_id = deepblue_get_regions(query_id =data_id,
                                 output_format = "CHROMOSOME,START,END")
request_data = deepblue_download_request_data(request_id)
deepblue_liftover(request_data, source = "hg38", target = "hg19")

Note that this was also the version of the if() from a few days back before our package build broke: https://github.com/mlist/rtracklayer/commit/a3c4308d928290700c6326eabda8dea4a96146e3

lshep commented 7 years ago

It would be great if this could be looked into as it is also affecting AnnotationHub.

lawremi commented 7 years ago

I don't think that above code tests the code in the error case. I will go ahead and merge this and then fix it.