-
Notifications
You must be signed in to change notification settings - Fork 20
CNV tools: archive 3 tools and related tests #1130
base: master
Are you sure you want to change the base?
Conversation
|
For reference, here are the tools we agreed to archive: DetectCoverageDropout I took what appeared to be related code and moved them. I did not check for other tool dependencies. |
|
I'll fix the imports, etc. and push commits to this branch. |
|
Some hiccups due to travis issues with sudo: https://www.traviscistatus.com/incidents/bnt2wtxpgs39 Restarted tests, I believe they should pass. |
|
@sooheelee @samuelklee @vdauwera I'd propose using a new program group 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 :) |
|
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. |
|
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. |
|
@samuelklee @droazen @LeeTL1220 Do any of you feel strongly we shouldn't keep this code around? |
|
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. |
|
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). |
|
@sooheelee Migration instructions for this branch: https://github.com/broadinstitute/gatk/wiki/Migrating-branches-from-gatk-protected-to-gatk |
|
Thank you @droazen. Will follow instructions. |
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