CNV CalculateTargetCoverage: tag doc; example WES command; change defaults

[sooheelee]

Example command is only for WES data. For WGS, I point users to SparkGenomeReadCounts.

• I nudge users to use PadTargets to pad their targets and tell them the target intervals do not overlap. I also state NOT to use BED format and point them to ConvertBedToTargetFile.
• I leave in the optional reference to safeguard newbie users. I do state that

  The reference is optional if --disableSequenceDictionaryValidation is set to true.

• description for targets param changed to specify 1-based indexing.
• Default params changed for: --groupBy SAMPLE --transform PCOV --targetInformationColumns FULL

[sooheelee]

Given changing default parameters cause integration tests to fail, and the less than two week time frame I have been given to document 40-some tools in gatk-protected, and the presumed unavailability of developers to then fix the integration tests (either the test values or the compared to results), it may be best if I do NOT change any default parameters going forward for these tools. @vdauwera?

---

cnorman [3:03 PM] The travis tests for your CalculateTargetCoverage branch are failing (the tests run automatically when you submit a branch or make a PR). I'd guess more than likely its due to the changes to the defaults.

[3:03] since its the tests for that tool that are failing

shlee [3:04 PM] Do you mean the integration tests or unit tests?

[3:04] Do I need to change the test parameters?

cnorman [3:05 PM] I don't know - there ar 10 failing - not sure if they're all integration tests

shlee [3:06 PM] Because I have set the defaults to allow the most parsimonious example command, which in turn is based on recommended parameters.

cnorman [3:06 PM] I think whoever owns the tool/tests needs to look at them to see what needs to be done

[3:07] Yes, but when the tests run, the check the results, and changing the default values can change the results of running the tool. So most likely the test checks now fail since the results are not what is expected.

shlee [3:07 PM] I see. So if for the test commands we call for the parameters that were previously default, then these tests would again succeed.

[3:08] Or we need to change the compared to results

cnorman [3:08 PM] exactly.

shlee [3:08 PM] Since it makes sense to test the most used parameters.

cnorman [3:09 PM] thats up to the dev (which way they want to make the tests work). But either way the change can't go in until the tests succeed

shlee [3:10 PM] I will talk to the devs

[sooheelee]

After discussing with Hellbender team, removed reference from the command.

[sooheelee]

After discussion with DR and Geraldine, decided going forward, at least for the next two weeks, not to change any more default parameters.

However, CalculateTargetCoverage's integration test remains broken because of the changed default parameters. This is the only tool that needs a fix in the integration test during this doc update @samuelklee @mbabadi @achevali.

[vdauwera]

Recommend changing the defaults back to the original value in that tool.

[sooheelee]

Ok, will do.

[LeeTL1220]

@sooheelee The automated tests are still failing. I'll send out for review when that is fixed.

[sooheelee]

@LeeTL1220 checks are passing now.

[davidbenjamin]

@sooheelee I can review this.

[davidbenjamin]

@sooheelee Back to you.

[sooheelee]

@davidbenjamin I've incorporated your feedback and answered questions. Thank you for reviewing.

[davidbenjamin]

@sooheelee looks good!

[sooheelee]

@samuelklee

discuss the issue with the default parameters and integration tests in person, it should be an easy fix.

Sounds good.

[sooheelee]

@samuelklee I will merge this now unless you envision the fixes to be a part of this PR?

[samuelklee]

Go ahead and merge!