salmonEC and alevinEC updates and tests

thelovelab / fishpond

Differential expression and allelic analysis, nonparametric statistics

https://thelovelab.github.io/fishpond

27 stars 9 forks source link

salmonEC and alevinEC updates and tests #21

Closed jgilis closed 2 years ago

jgilis commented 2 years ago

Hi Mike,

I have updated the code for salmonEC.R and alevinEC.R, and included tests for both. I here summarize the changes:

both functions now handle faulty/incomplete tx2gene entries, in a way that is very similar to what you do in tximport
in line with that, I added the ignoreTxVersion and ignoreAfterBar arguments
the output is no longer a sparse count matrix, but a list of 2. The first element of that list is the original sparse count matrix. The 2nd argument is a dataframe that allows for linking equivalence classes to transcripts and genes (see the details section of the documentation of the function)
I added test for both functions, that pass R cmd and BiocChecks on my machine.
The tests are quite extensive, let me know if they take to much time then I could cut down to the most important ones
There is 1 conflict, but its just the package version ;)

Best regards,

Jeroen

mikelove commented 2 years ago

I merged this on the command line, unfortunately it seems to have put the commit lines to me instead of you, but I followed GitHub's instructions :(

I'll check the tests on my end

mikelove commented 2 years ago

Currently the tests for existing fishpond functions in devel branch are in total ~30 seconds. Adding test_salmonEC as is adds ~30 seconds on my machine, and adding test_alevinEC adds ~40 seconds on my end. I propose to trim down some of the longer tests, putting them behind an optional flag slow, so you can still run them if you need to detect regressions, but they won't be run every time the package is checked. This brings test_salmonEC to 9 seconds, and test_alevinEC to 10 seconds which is fine, and both are still testing the main functionality and all the errors. I just pushed this new version, so hopefully that's ok with you. I leave all the error tests that don't affect the runtime. Finally, I put everything within a test on tximportData package version so GHA still work.

jgilis commented 2 years ago

Yes that all sounds fine to me. I think keeping the tests light but having some additional ones as backup when issues come up is the perfect scenario! Its also nice to see that the new code has converged to something more closely related to tximport in terms of input arguments, tx2gene handling and output format.

mikelove commented 2 years ago

Great, thanks again for the contribution! Good luck with the writeup :)