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

Making MAF become the output of Funcotator in M2 WDL and multiple transcript fix. #4941

Merged
merged 15 commits into from
Jul 16, 2018

Conversation

LeeTL1220
Copy link
Contributor

@LeeTL1220 LeeTL1220 commented Jun 22, 2018

@codecov-io
Copy link

codecov-io commented Jun 22, 2018

Codecov Report

Merging #4941 into master will increase coverage by 0.002%.
The diff coverage is 94.286%.

@@              Coverage Diff               @@
##              master    #4941       +/-   ##
==============================================
+ Coverage     80.468%   80.47%   +0.002%     
- Complexity     17852    17856        +4     
==============================================
  Files           1093     1093               
  Lines          64295    64302        +7     
  Branches       10378    10377        -1     
==============================================
+ Hits           51737    51744        +7     
  Misses          8504     8504               
  Partials        4054     4054
Impacted Files Coverage Δ Complexity Δ
...cotator/mafOutput/CustomMafFuncotationCreator.java 90% <100%> (ø) 17 <1> (ø) ⬇️
.../tools/funcotator/mafOutput/MafOutputRenderer.java 95.908% <100%> (+0.042%) 59 <9> (+2) ⬆️
...dataSources/gencode/GencodeFuncotationFactory.java 83.383% <80%> (+0.075%) 162 <6> (+2) ⬆️

@LeeTL1220 LeeTL1220 force-pushed the ll_4935_Funcotator_MAF_WDL branch from d4a9471 to 40341bb Compare June 22, 2018 21:03
@LeeTL1220 LeeTL1220 force-pushed the ll_4935_Funcotator_MAF_WDL branch from 9150721 to 5fa68f9 Compare July 3, 2018 13:22
@LeeTL1220 LeeTL1220 changed the title Making MAF become the output of Funcotator in M2 WDL. (Do not merge yet) Making MAF become the output of Funcotator in M2 WDL and multiple transcript fix. (Do not merge yet) Jul 5, 2018
@LeeTL1220 LeeTL1220 requested a review from davidbenjamin July 5, 2018 20:35
@LeeTL1220 LeeTL1220 changed the title Making MAF become the output of Funcotator in M2 WDL and multiple transcript fix. (Do not merge yet) Making MAF become the output of Funcotator in M2 WDL and multiple transcript fix. Jul 5, 2018
@LeeTL1220
Copy link
Contributor Author

@davidbenjamin
This will make the WDL default to producing a MAF from Funcotator instead of a VCF. There is no flag to switch between the two, so if you know of people that still want VCF output, please speak up now...

Can you review the WDL and autotest-WDL changes? This has been tested in FireCloud and looks good (minus an issue that I have already filed), though I had to manually review. The Method configuration in FireCloud still uses the GATK jar override for this. Just in case you wanted to run it. Otherwise, we should blank out that parameter.

As a reminder, I tested:

  • mutect2.wdl: manually on local backend and FireCloud
  • mutect2_nio.wdl: manually on FireCloud

Please review both WDL files.

@jonn-smith Could you review the rest? I.e. the bug fixes.

Apologies that I did not split these into two PRs.

@LeeTL1220
Copy link
Contributor Author

@davidbenjamin @jonn-smith The test failure is a false alarm. Please review as if the tests were passing.

@davidbenjamin davidbenjamin self-assigned this Jul 12, 2018
Copy link
Contributor

@davidbenjamin davidbenjamin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm satisfied.

String annotation_over_arg = if defined(annotation_overrides) then " --annotation-override " else ""
String filter_funcotations_args = if (filter_funcotations) then " --remove-filtered-variants " else ""
String interval_list_arg = if defined(interval_list) then " -L " else ""
String extra_args_arg = select_first([extra_args, ""])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as just $extra_args

Copy link
Contributor Author

@LeeTL1220 LeeTL1220 Jul 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(in mutect2_nio.wdl as well)

String annotation_def_arg = if defined(annotation_defaults) then " --annotation-default " else ""
String annotation_over_arg = if defined(annotation_overrides) then " --annotation-override " else ""
String filter_funcotations_args = if (filter_funcotations) then " --remove-filtered-variants " else ""
String interval_list_arg = if defined(interval_list) then " -L " else ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This accomplishes the same as just ${"-L " intervals}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

${"-L " + interval_list} Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(in mutect2_nio.wdl as well)

Boolean use_ssd = false

# This should be updated when a new version of the data sources is released
# TODO: Make this dynamically chosen in the command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was out of date and has been eliminated in favor of a reasonable default. Also, we will probably remove the FTP download at some point, since it often breaks.

Done

@@ -27,7 +27,7 @@
*/
public class CustomMafFuncotationCreator {

final static List<String> COUNT_FIELD_NAMES = Arrays.asList(
final public static List<String> COUNT_FIELD_NAMES = Arrays.asList(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public final

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -220,6 +220,10 @@ public MafOutputRenderer(final Path outputFilePath,

@Override
public void close() {
if (!hasWrittenHeader) {
final LinkedHashMap<String, String> mafCompliantOutputMap = createMafCompliantOutputMap(Allele.create("AT"), Collections.emptyList());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is "AT"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put in some clarifying comments.

Done.

@LeeTL1220 LeeTL1220 merged commit 1977c53 into master Jul 16, 2018
@LeeTL1220 LeeTL1220 deleted the ll_4935_Funcotator_MAF_WDL branch July 16, 2018 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants