search

snakemake-workflows / rna-seq-kallisto-sleuth

A Snakemake workflow for differential expression analysis of RNA-seq data with Kallisto and Sleuth.
MIT License
66 stars 44 forks source link

fix: Handle missing bam columns in units.tsv #105

Closed fxwiegand closed 3 months ago

fxwiegand commented 3 months ago

This PR makes the workflow handle missing bam columns introduced in #94 without throwing an error.

fxwiegand commented 3 months ago

@dlaehnemann Just as we discussed in #104.

I removed the columns from the test config so the CI should show us if the workflow handles the missing columns. I dont even thing we need to set a default value in the lookup() functions as these rules will only be executed if the columns are present in the units sheet. I only added a None default value to the lookup() functions as they will only be executed when the value is present in the units sheet anyways.

dlaehnemann commented 3 months ago

I just realised, that we also do not have a test case for the new bam input functionality, and we probably should. Otherwise things are sure to break at some point. I see two possibilities, here:

  1. Extend what we are doing in the QuantSeq tests right here, by introducing a rule that maps one pair of files and then we use the resulting bam file in our units.tsv as another input.
  2. Include some bam file by mapping the existing fqs in the separate ngs-test-data repo and then write up a test case based on that?

Or maybe you know a (very minimal) bam file that we can directly use, optimally in some stable repository for download.

And it probably makes sense to include these tests in this PR, as this is the functionality that it should test...

fxwiegand commented 3 months ago

I agree that a test case with the bam input would be helpful although really all it will test is the modified fastq input function as well as the samtools separate and interleaved wrapper (that should already be tested well enough).

I would argue to merge and release this now and add a test for the bam input later as this will at least allow any users to use the newest workflow version with the regular fastq inputs without any breaking changes.

fxwiegand commented 3 months ago

@dlaehnemann It seems like the suggestions caused some new bug 😬

Edit: Ah i think i found the issue. Two of the suggestions were missing a simple not.

dlaehnemann commented 3 months ago

Oups. :see_no_evil: