New features, bug fixes, and new test case framework for 4.16.0

[welchr]

RAREMETAL

New features:

• Heterogeneity statistics, mean/se/min/max allele frequency tracking (Output additional statistics to support QC #20)
• Run single variant tests w/o aggregation tests (Run single variant meta-analysis without aggregation tests #24)
• Increase p-value precision for single variant tests, and optional log-pvalue output (Increase p-value precision #23)
• New test case framework (Catch2) + test cases (cc6edc2)
• Travis CI and status checks on master/develop branches (Add travis & status checks for master, develop branches #29)
• Dockerfile for local development / testing linux builds (ff5e1dc)

Bug fixes:

• Incorrect handling of dosage counts when determining if variant is monomorphic (Truncation from double to integer results in skipping some singleton/rare variants #4)
• Running in a region with --range chr:start-end limits output to that region for both aggregation tests and single variant tests, previously only aggregation tests were limited (Run single variant and aggregation tests only within region #26)
• Fix compile error on linux (bc976dd)

RAREMETALWORKER

Bug fixes:

• Program would terminate normally even though input file was not entirely read when a multi-allelic variant was present (Multiallelic variant causes rest of VCF to be skipped #7)

[welchr]

@abought - did you want to look at anything in here before I merge into develop?

[abought]

Hi Ryan! Thanks for all the (tremendous) work on this.

20,000 lines is quite a lot to take in all at once for feedback!

Two initial speedbumps:

1. I'm having some trouble compiling on Mac OS 10.14: it seems to require a savvy upgrade but I get errors about xz not found. Do we need to document any dependency changes in the README?

2. At this instant in time, there doesn't seem to be a single consolidated definition of what constitutes "passing release criteria". Can we update the repo and docs?

• Based on the current travis.yml, the old integration tests still exist in the repo, but aren't being run. This creates a grey area where they might be relevant- or not. Can we either verify they are passing (via CI), or remove the tests as a dead end? From the old README: To run unit tests, ensure that you have run cmake with -DBUILD_TESTS=1 and built at least once, then run make or ctest --verbose. (** I think that should say "make test"?)

• Based on travis.yml, there are some new commands required to run unit tests, and these commands are different from what is documented in the README.

[jonathonl]

Apple decided (for whatever reason) to prevent /usr/include directories on recent versions of MacOS, which is breaking some autotools projects. The xz dependency actually is not required, and you should be able to use cget ignore xz to get around this.

[welchr]

I've noticed a few times too that when cget runs and tries to download certain tarballs (like xz) it will randomly fail, and I just have to re-run it again. When you run cget install -f requirements.txt is it failing to grab xz, or is it failing to install entirely?

[welchr]

Updates so far:

• I think we've got the compiling issues sorted out, and travis is updated to test for Mac OS 10.14 and 10.15 (thank you Jonathon for all the help here. 👍) I added some helpful info to the README about this as well.

• The older raremetal test cases are removed, and I updated the README with the relevant test commands for running the new test suite.

One lingering problem, unfortunately:

• I created a branch where I tried adding in the old raremetalworker test cases to travis. Unfortunately, one of the older raremetalworker test cases only passes on MacOS, but not on Linux. There is a slight numerical precision difference on the two platforms and that causes enough of a diff that it fails (I confirmed this with a Linux docker image outside of Travis.)

  I think I'm inclined to let this go, since there's a fair chance that raremetalworker will be deprecated in favor of rvtest in the future, and this PR doesn't change the status quo at least (the test would never have worked in Linux, and no code changes here touch raremetalworker code.)

  To fix it would require the same work needed in raremetal - refactor everything to remove global state(s), add in supporting code for loading and comparing results, and then add in the test cases as well. That's quite a bit of work. I'm not sure it would be time well spent at this point, either.

  What do you guys think?

[abought]

I can confirm that the precision difference is the cause of the testcase failure: this was one of the big drawbacks to the old diff-output-files approach. Very glad to see real tests (and Travis) added; thanks both!

If we actively plan to remove RAREMETALWORKER, then getting rid of those tests is a plausible option. I don't have as much experience with ctest, but I assume that the tests could conditionally only be run on a specific platform?

With our unpredictable schedules, we don't always get around to removing code as planned, and then years later a student assumes it's safe to change. If we could keep the tests (where relevant) without too much effort, this might be a middle ground where we avoid the drawbacks, but keep the safety net.

Either way: when the code really is removed, go wild and remove the dead tests too.

[welchr]

Thanks Andy that made good sense to me. I made the old raremetalworker tests only run under OSX in Travis, so that should prevent future build failures. They're still in the repo too in case we end up potentially working on raremetalworker in the future.

[abought]

As reported in an email from Sarah Graham:

I did catch a bug in this version (fix/welchr), when the condition option is used, the header gets the last column twice:
"#CHROM POS REF ALT N POOLED_ALT_AF DIRECTION_BY_STUDY EFFECT_SIZE EFFECT_SIZE_SD H2 PVALUE COND_EFFSIZE COND_EFFSIZE_SD COND_H2 COND_PVALUE COND_PVALUE"