broadinstitute / gatk-protected

Obsolete/Legacy GATK repository -- go to https://github.com/broadinstitute/gatk instead
BSD 3-Clause "New" or "Revised" License
33 stars 20 forks source link

CNV tools: archive 3 tools and related tests #1130

Open sooheelee opened 7 years ago

sooheelee commented 7 years ago

Nine files were moved to new folder called src/main/java/org/broadinstitute/hellbender/tools/archive/:

@samuelklee @LeeTL1220 Please let me know if some of these need to be retained in the active code base.

FYI--@vdauwera @cmnbroad

sooheelee commented 7 years ago

For reference, here are the tools we agreed to archive:

DetectCoverageDropout DecomposeSingularValues CalculatePulldownPhasePosteriors

I took what appeared to be related code and moved them. I did not check for other tool dependencies.

samuelklee commented 7 years ago

I'll fix the imports, etc. and push commits to this branch.

samuelklee commented 7 years ago

Some hiccups due to travis issues with sudo: https://www.traviscistatus.com/incidents/bnt2wtxpgs39 Restarted tests, I believe they should pass.

droazen commented 7 years ago

@sooheelee @samuelklee @vdauwera I'd propose using a new program group ArchivedToolsProgramGroup for the archived tools, as physically moving them into an archive directory does not really accomplish much (the tools will still show up in the --help output under the CNV category, in this case).

Also, GATK3 had a terrible tradition of "archiving" code in a directory outside of the source tree, where it quickly became uncompilable, and I want to make sure that we're not starting down that path here. If you think about it, the git history itself is a pretty good "archive" of sorts :)

samuelklee commented 7 years ago

Sounds good to me, @droazen. I would not be completely averse to just removing the code, either, if it's awkward to have a separate group just for these tools.

vdauwera commented 7 years ago

Yeah we definitely don't want to repeat the mistakes of the past. I do think we need a reasonable system to "physically" declutter the code by moving the archived code out of the way, though not outside the source tree. The new program group makes sense to me.

I'm reluctant to rely solely on git history as an archive, but can be persuaded if you feel strongly that we shouldn't keep this code around.

sooheelee commented 7 years ago

@samuelklee @droazen @LeeTL1220 Do any of you feel strongly we shouldn't keep this code around?

samuelklee commented 7 years ago

My vote is for ditching it. Even if this code is cordoned off into a separate program group, changes in code dependencies may require us to continue to maintain it. And if the program group remains visible, comms may get questions about these tools, adding to their support burden.

In my opinion, the less overhead, the better! We can always come back to the git history in the extremely unlikely event that we'll need to reinstate these tools in the future.

vdauwera commented 7 years ago

Hiding the program group in external docs is trivial, so that shouldn't be an important factor in the decision. The code maintenance burden argument is much more convincing to me.

Given that, I'm ok with deleting the code entirely but I would request that the PR and commit message specify whether there is a replacement for each of the tools. In addition there should be a deprecation message so that if I try to run one of these tools in a newer version, I get a helpful error message that tells me the tool was removed and by what it was replaced if applicable. See GATK3 for how we implemented this previously. This should be done for all tools that we remove, regardless of whether they were purely internal or experimental. It's only a one line addition per tool and it can potentially save us a lot of headaches later down the road (even just internally).

droazen commented 7 years ago

@sooheelee Migration instructions for this branch: https://github.com/broadinstitute/gatk/wiki/Migrating-branches-from-gatk-protected-to-gatk

sooheelee commented 7 years ago

Thank you @droazen. Will follow instructions.