-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Handle missing bam columns in units.tsv #105
Conversation
@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. |
Co-authored-by: David Laehnemann <david.laehnemann@hhu.de>
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:
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... |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point for making this available quickly, as this un-breaks things. Let's think about a simple further test in a separate PR.
Also, the current version works fine, but I do have one suggestion to make it a bit less repetitive (and maybe a bit more intuitive to read). Feel free to use or not.
Co-authored-by: David Laehnemann <david.laehnemann@hhu.de>
Co-authored-by: David Laehnemann <david.laehnemann@hhu.de>
Co-authored-by: David Laehnemann <david.laehnemann@hhu.de>
@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 |
Oups. 🙈 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To miss, or not
to miss, that is the column.
🤖 I have created a release *beep* *boop* --- ## [2.7.0](v2.6.0...v2.7.0) (2024-08-15) ### Features * Improve datavzrd tables ([#93](#93)) ([93512b8](93512b8)) ### Bug Fixes * Fix missing output in spia.R when no significant genes are found ([#103](#103)) ([bc0d017](bc0d017)) * Handle missing bam columns in units.tsv ([#105](#105)) ([bae88d0](bae88d0)) * Remove non-existent outputs in spia rule ([#102](#102)) ([0fbb930](0fbb930)) * update to latest datavzrd ([417ec3b](417ec3b)) ### Performance Improvements * datavzrd wrapper `v3.12.1`, offer-excel configurable, free disk space for CI, dynamic sleuth_init mem_mb, pure download rules as localrules ([#92](#92)) ([70850fb](70850fb)) * Update datavzrd wrapper ([#98](#98)) ([e5eb0e0](e5eb0e0)) * Update samtools fast separate wrapper ([#100](#100)) ([65d8f41](65d8f41)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR makes the workflow handle missing bam columns introduced in #94 without throwing an error.