Skip to content
This repository was archived by the owner on Nov 9, 2019. It is now read-only.

Conversation

@sooheelee
Copy link
Contributor

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

  • CalculatePulldownPhasePosteriors.java
  • CoverageDropoutDetectorTest.java
  • DecomposeSingularValuesIntegrationTest.java
  • CalculatePulldownPhasePosteriorsIntegrationTest.java
  • CoverageDropoutResult.java
  • DetectCoverageDropout.java
  • CoverageDropoutDetector.java
  • DecomposeSingularValues.java
  • DetectCoverageDropoutIntegrationTest.java

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

FYI--@vdauwera @cmnbroad

@sooheelee
Copy link
Contributor Author

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
Copy link
Contributor

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

@samuelklee
Copy link
Contributor

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

@droazen
Copy link
Contributor

droazen commented May 31, 2017

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@samuelklee
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

droazen commented Jun 2, 2017

@sooheelee
Copy link
Contributor Author

Thank you @droazen. Will follow instructions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants