Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed BetaFeature tag from Mutect2 and FilterMutectCalls #4384

Merged
merged 1 commit into from
Feb 15, 2018

Conversation

davidbenjamin
Copy link
Contributor

@chandrans Can I give this one to you?

@davidbenjamin davidbenjamin added this to the Popularize Mutect 2 at the Broad milestone Feb 9, 2018
@droazen
Copy link
Contributor

droazen commented Feb 9, 2018

@LeeTL1220 please also review

@codecov-io
Copy link

Codecov Report

Merging #4384 into master will increase coverage by 0.005%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##              master     #4384       +/-   ##
===============================================
+ Coverage     79.093%   79.098%   +0.005%     
  Complexity     16666     16666               
===============================================
  Files           1050      1050               
  Lines          59698     59698               
  Branches        9756      9756               
===============================================
+ Hits           47217     47220        +3     
+ Misses          8693      8691        -2     
+ Partials        3788      3787        -1
Impacted Files Coverage Δ Complexity Δ
...bender/tools/walkers/mutect/FilterMutectCalls.java 95.833% <ø> (ø) 7 <0> (ø) ⬇️
...itute/hellbender/tools/walkers/mutect/Mutect2.java 92% <ø> (ø) 15 <0> (ø) ⬇️
...park/sv/discovery/alignment/AlignmentInterval.java 90.038% <0%> (+0.383%) 74% <0%> (ø) ⬇️
...e/hellbender/engine/spark/SparkContextFactory.java 73.973% <0%> (+2.74%) 11% <0%> (ø) ⬇️

@LeeTL1220
Copy link
Contributor

@davidbenjamin What is the criteria met here?

@chandrans
Copy link
Contributor

@LeeTL1220 Hey Lee. There was some confusion, and my team thought Mutect2 is still in beta. Removing the beta tag will allow our docs to show the tool as not in beta. Simple fix :)

@droazen
Copy link
Contributor

droazen commented Feb 9, 2018

@chandrans Untagging a tool as beta is a major step that should not be taken lightly, since it can't be reversed (we can't mark tools as beta again once they've been unmarked, as @vdauwera often reminds us). I think @LeeTL1220 is asking whether the latest master version of Mutect2 has been run through the necessary evaluations that would justify removing the beta label.

@droazen
Copy link
Contributor

droazen commented Feb 12, 2018

I'll also add here: if existing well-known arguments are going to be removed from Mutect2 in the near future, as --dbsnp apparently was just recently (see #4390), then it might be a good idea to keep it tagged as @BetaFeature for now. Once you remove the beta label, you can't really remove widely-used tool arguments without deprecating them for a period of time first (as I think @vdauwera would agree).

@chandrans
Copy link
Contributor

Ah, I see. My major push for this to be out of beta is that users are confused and ask about the timeline for Mutect2 to be out of beta status. My team was also confused thinking Mutect2 is still in beta because of the tag. From my position, the tool should be "not in beta" if the users can use it in production. I think the beta tag makes people hesitant to use it for proper analysis even if it is production-ready. However, I leave it to David B and team to determine whether there will be major changes in the code that would make this difficult.

@davidbenjamin
Copy link
Contributor Author

I'm for taking M2 out of beta because being in beta leaves users to the much worse alternatives of using a different Mutect. @droazen has a good point about changing arguments, but in the case of dbsnp it was an argument that we haven't used in over a year. I should have given it a deprecated tag long ago but forgot to do so. I don't foresee taking anything out of the command line until we get tagged arguments, at which point we will remove the -tumor and -normal arguments.

@droazen
Copy link
Contributor

droazen commented Feb 14, 2018

@chandrans @davidbenjamin I'm not actually objecting to the idea of taking Mutect2 out of beta, for the record. I'm just trying to get people used to the idea that removing the @BetaFeature tag is actually a "big step" that requires careful evaluation, since it can't be undone. It's not just something that affects tool documentation -- it affects our entire release and development process now that we're out of beta. If a tool is marked stable, it needs to be kept in a constantly releasable state in master, and major changes to stable tools need to be done in such a way that existing functionality is not compromised.

If the latest master version of Mutect2 has been run through and passed whatever evaluation criteria/scripts your team relies on @davidbenjamin, and you are comfortable at this point with the additional restrictions that come with doing development on a stable tool, then by all means take it out of beta!

@davidbenjamin
Copy link
Contributor Author

@LeeTL1220 The criteria in my opinion are being the best Mutect and being stable. Were you suggesting waiting for some validation like MC3?

@LeeTL1220
Copy link
Contributor

@davidbenjamin This decision is yours. I will support it either way.

@davidbenjamin
Copy link
Contributor Author

Okay, I'm going to do it!

@davidbenjamin davidbenjamin merged commit ddd0aaa into master Feb 15, 2018
@davidbenjamin davidbenjamin deleted the db_m2_is_not_beta branch February 15, 2018 02:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants