search

Bioconductor / Rsamtools

Binary alignment (BAM), FASTA, variant call (BCF), and tabix file import
https://bioconductor.org/packages/Rsamtools
Other
27 stars 27 forks source link

example(applyPileups) ... code works #26

Closed vjcitn closed 3 years ago

vjcitn commented 3 years ago

the example is in a \dontrun{} but when pasted in, the code completes. are the results simply wrong?

mtmorgan commented 3 years ago

git blame points to this commit 8ac1df19 with unit tests failing.

I believe one wants to use pileup instead.

vjcitn commented 3 years ago

OK, I ran the TEMPORARY_DISABLED tests on m1 mac and all passed. Maybe deprecate applyPileups? The back story is that I was looking for an example to run in docker run -ti quay.io/biocontainers/bioconductor-rsamtools:2.8.0--r41h399db7b_0 bash

hpages commented 3 years ago

Yeah let's reduce the technical debt by getting rid of applyPileups.

For the context I had to "temporarily" disable the applyPileups examples and unit tests back in 2019 when I was migrating Rsamtools from samtools 0.19 to htslilb 1.7 because they were causing problems (can't remember the details). This migration process required porting/adapting a lot of C code that I didn't write initially. I was able to do it for the most part but in some cases like applyPileups something was not working properly but I decided to postpone the fix so we could have the new Rsamtools out as soon as possible (this migration took about 2 months). I was also under the impression that applyPileups was superseded by pileup so was not too eager to spend too much time on fixing something obsolete.

hpages commented 3 years ago

@mtmorgan: So is it ok to deprecate applyPileups?

mtmorgan commented 3 years ago

yes

hpages commented 3 years ago

Done: 2b0a778d751a66d375568764e09ee69cbf7fc4c5