Find ConvertSequencingArtifactToOxoG files properly. Fix for indexcov issue. #1310
Conversation
INPUT_BASE is the switch that is used, not INPUT for this tool. (see https://gatk.broadinstitute.org/hc/en-us/articles/360037058092-ConvertSequencingArtifactToOxoG-Picard-) These files were getting missed without this tweak.
Closes MultiQC#1265 and bcbio/bcbio-nextgen#3350. Thanks to @fgviera and @WimSpree for pointing the issue out.
roryk
changed the title
|
Sorry for not breaking these up into two branches, hope that is okay. Both fixes tested and fix their respective issues. |
|
Heya, just a ping for this, sorry to be a pain-- there are two fixes here that are pretty simple and fix a crash plus fix an issue with some metrics not showing up. Is there anything I can do to help to get them merged in? |
| context_col = None | ||
|
|
||
| # Pull sample name from input | ||
| fn_search = re.search(r"INPUT_BASE(?:=|\s+)(\[?[^\s]+\]?)", l, flags=re.IGNORECASE) |
There was a problem hiding this comment.
I don't have any example data in ewels/MultiQC_TestData which has ConvertSequencingArtifactToOxoG in it so I can't test this. But I think that your change is basically to allow either INPUT or INPUT_BASE in the regex, is that right? In which case I'm not sure that we need to duplicate this whole code block. I think that we can just add an optional non-capturing group to the regex to allow either.
|
Sorry for the delay in reviewing. I am snowed under with MultiQC at the moment and struggling to keep up with PRs (I have a plan to address this, just no time to implement it - a bit of a catch 22 situation!). Anyway, as this was a small PR I'll try and sneak it in before this release. I don't have appropriate test data to really try out the new code (if you fancy making a PR to ewels/MultiQC_TestData to address this that would be fab). But I think that my edits to your PR should have the same effect and make the PR even smaller 😄 I'll merge after the tests pass so as not to keep you waiting, but please to check that you agree with my changes and let me know if you find any problems. Phil |
Hi Phil! No worries, I am sorry you are feeling overwhelmed with multiqc, I totally understand how that happens. It is a curse to be popular. Please let the community know if we can help out in any way, multiqc is really popular and useful and I am sure there are a bunch of people who will be willing to pitch in. Yup, your changes look great. Thank you. I'll p/r the test file tomorrow morning, let me track one down. |
|
Thanks for checking! My plan is basically to decentralise the modules into separate packages with their own releases and maintainers. So at some point I will be on the hunt for people willing to keep an eye on specific MultiQC modules for incoming PRs and bug reports. If that's something you think you might be able to help with then keep your eye out on twitter / this repo etc and something should appear one day.. 😄 |
INPUT_BASE is the switch that is used, not INPUT for
this tool. (see
https://gatk.broadinstitute.org/hc/en-us/articles/360037058092-ConvertSequencingArtifactToOxoG-Picard-)
These files were getting missed without this tweak.
CHANGELOG.mdhas been updated