tidyomics / plyranges

A grammar of genomic data transformation
https://tidyomics.github.io/plyranges/
140 stars 18 forks source link

shift_downstream() doesn't work with vectors of same length as GenomicRanges object as the shift value #73

Closed Bostrolicious closed 4 years ago

Bostrolicious commented 4 years ago
gr <- GRanges(seqnames="chr1", strand=c("+", "-", "+"), ranges=IRanges(start=c(3, 8, 11), width=2))
plyranges::shift_downstream(gr, c(3, 3, 3))

yields the error message:

Error in .normarg_shift(shift, length(x)) : 
  'length(shift)' is greater than 'length(x)'

The same error message displays even if the shift vector is shorter than x (and not of length 1). The same is true for shift_upstream(). shift_left() and shift_right() both work fine. Encountered using plyranges version 1.6.2.

sa-lee commented 4 years ago

Thanks for the report, it should be fixed now - will push through shortly to release branch

Bostrolicious commented 4 years ago

Great, thanks for the quick fix! I recently started using plyranges, and it's been excellent so far.

While on the topic of these functions, is there any reason to disallow 0 and negative values for the shift parameter? When supplying a vector for the shift value, I think it would often be useful to shift ranges in different directions with the same function. Similarly, sometimes not all ranges need to be shifted, in which case allowing 0 would be useful. For a case where I would want one range to shift upstream, one to remain where it is, and one to shift downstream, being able to do that with shift_downstream(gr, c(-1, 0, 1)) would be very handy.

Apologies if this isn't the right place for a suggestion - I didn't know if a new issue was warranted as it's not a bug and perhaps not a good idea to implement.

sa-lee commented 4 years ago

So zeros are accepted (this was always intended but there was a bug in my checks). One of our thoughts around not allowing for negative values was to make it transparent what's being shifted upstream/downstream, if you supply a negative value in a sense the function is lying to you about its intent (as one range will be upstream). In your case two calls can achieve the same result:

gr %>%
    shift_downstream(c(0,0,1)) %>%
    shift_upstream(c(1,0,0))

If you want to do everything in one call take a look at IRanges::shift()

Bostrolicious commented 4 years ago

That makes sense. As long as zeros are accepted, using two shift commands in succession is easy, and I understand your rationale for disallowing negative values. Without zeros, I had to split my GRanges object in three different variables by which shift direction (-/0/+) was required and then apply shift functions.

Thanks!