Allows BAM with csi index to be used #58
Conversation
| elsif ( -e "${path}.crai" && mtime($path) > mtime("${path}.crai") ) { | ||
| $self->reindex($path); | ||
| } | ||
| } | ||
|
|
||
| croak "No index file for $path; try opening file with -autoindex" | ||
| unless -e "${path}.bai" or | ||
| -e "${path}.crai"; | ||
| -e "${path}.csi" or | ||
| -e "${path}.crai"; |
There was a problem hiding this comment.
Aren't you actually suggesting to fail in the csi case by telling to rerun with autoindex?
There was a problem hiding this comment.
I'm assuming at some point -autoindex will allow csi.
| @@ -514,6 +514,15 @@ for my $use_fasta ( 0, 1 ) { | |||
| ok( $matches{matched}/$matches{total} > 0.99 ); | |||
| } ## end for my $use_fasta ( 0, ...) | |||
|
|
|||
| { # test access via csi index | |||
There was a problem hiding this comment.
This is fine, but for complete coverage, I would do something similar to the low/high level tests performed on ex1 to check whether certain operations with the csi index work as expected.
There was a problem hiding this comment.
I can look into extending the tests if it's the only way to get this moving. I only created the branch as we were getting no response or idea when something would happen.
There was a problem hiding this comment.
I've made the comment, but haven't actually requested the mandatory change. Sorry if you feel communication isn't what you expect. Rishi has just recently handed over maintenance to the e! core team (specifically me). I'm gradually getting my way through this, but this task is not the priority of my job at the moment.
There was a problem hiding this comment.
I'm currently adding more robust tests by changing the ex1 tests to be reused for the ex3.bam with csi index. Looking at this test class looks like I've written most of them over the years. I'll raise a new pr once I've finished them
Provide csi support for use of BAMs with csi already generated as discussed in #57.
I tested with HTSlib 1.7