Closed ChristinaXu2017 closed 2 years ago
There is a bug
package that has been added. Is this intentional?
There is a
bug
package that has been added. Is this intentional?
what is it?
There is a
bug
package that has been added. Is this intentional?what is it?
There are a number of classes in a package called bug
eg. qcoverage/bug/CoverageJobTest.java
that you have committed as part of this PR.
Thanks for detecting it. The "bug" is deleted now.
The current Coverage.saveCoverageReport
method does the following:
if (options.hasXmlFlag()) {
writeXMLCoverageReport(stats);
// if (options.hasPerFeatureOption())
// writeVCFReport(stats);
} else if (options.hasPerFeatureOption()) {
writePerFeatureTabDelimitedCoverageReport(stats);
} else {
writePerTypeTabDelimitedCoverageReport(stats);
}
whereas the version in this PR does:
if (options.hasXmlFlag()) {
writeXMLCoverageReport(stats);
}
if (options.hasTxtFlag() ) {
writePerFeatureTabDelimitedCoverageReport(stats);
}
so it looks like the output behaviour is changing. Is this intentional? If so, could you please update the description in this PR to record what the new behaviour is, and why it is better than the old behaviour? I presume that the documentation has also been updated?
Also, could you please perform a check on the output file extension. The wdl currently sets the output with the xml file extension and if this is not changed, then the actual generated output will have a double xml suffix which could impact downstream processes. Thanks
the pull request description is updated. Here we will throw exception if ask vcf output but not per-feature mode
Thanks for the fileNameCorrection
method in the Coverage
class. This will prevent duplication of file extensions.
A few suggestions - perhaps you could raise an issues for these?
I think that the method would be better placed as a static method in the FileUtils
class in the qcommon
package.
That way it could easily be used by other classes.
If its static, then it is not going to modify any state, which makes it referentially transparent.
Null guards should be put in place for the parameters.
And finally, a unit test should be added.
please make sure you make the necessary changes to the wdl files as soon as this is merged into master
Description
Overall, this tool options are updated and new usages show as below:
Please delete options that are not relevant.
How Has This Been Tested?
unit test is updated, tested on real data set
Are WDL Updates Required?
common/qcoverage.wdl and somaticDnaFastqToMaf.wdl will need to be updated to reflect new option names
Checklist: