feat: Adding tmb calculation #403
Conversation
There was a problem hiding this comment.
I think it is very good already, but there are a few things to be changed, see the comments below.
The code must be cleaned up to follow the formatting rules laid by Manuel. You need to use black (with line length 100) & flake8 on your code. The order of imports is checked with isort and snakefmt ensures consistent Snakefile format.
Finally, you will need to write tests for you step.
snappy_pipeline/apps/snappy_snake.py
Outdated
There was a problem hiding this comment.
I am going to be pedantic and ask you to change the name of the step. It should be tumor_mutational_burden, not tmb_calculation.
Likewise, the naming pattern for output files should be {mapper}.{caller}.tmb.{tumor_library} or {mapper}.{caller}.tumor_mutational_burden.{tumor_library}, but not {mapper}.{caller}.tmb_calculation_json.{tumor_library}.
(There are only two hard things in Computer Science: cache invalidation and naming things, Phil Karlton (from X11))
There was a problem hiding this comment.
The action is usually run. In steps with many stages (such as somatic variant calling, with the calling itself, the contamination estimation and filtering) the action can take different values. But is simple steps as tumor_mutational_burden, it should be run.
| config, lookup_paths, config_paths = expand_ref("config.yaml", config) | ||
|
|
||
| # WorkflowImpl Object Setup =================================================== | ||
| wf = TumorMutaionalBurdenCalculationWorkflow(workflow,config,lookup_paths,config_paths,os.getcwd()) |
There was a problem hiding this comment.
Typo: it should be TumorMutationalBurdenCalculationWorkflow.
| tools_somatic_variant_calling: [] # default to those configured for somatic_variant_calling | ||
| """ | ||
|
|
||
| class TumorMutaionalBurdenCalculationStepPart(BaseStepPart): |
There was a problem hiding this comment.
Typo: should be TumorMutationalBurdenCalculationStepPart
| """Calculation tumor mutational burden for each sample""" | ||
| name = 'tmb_gathering' | ||
|
|
||
| actions = ("tmb_gathering",) |
There was a problem hiding this comment.
actions should be ("run",)
| actions_str = ", ".join(self.actions) | ||
| error_message = f"Action '{action}' is not supported. Valid options: {actions_str}" | ||
| raise UnsupportedActionException(error_message) | ||
| mem_mb = 7 * 1024 * 2 #read |
There was a problem hiding this comment.
You are requesting too many resources: 1 core, 4GB & 1 hour should be enough
| tpl, tumor_library=[sample_pair.tumor_sample.dna_ngs_library], **kwargs | ||
| ) | ||
|
|
||
| def _yield_result_files_joint(self, tpl, **kwargs): |
There was a problem hiding this comment.
I don't think that you need to worry about joint callers. This method could go.
| path_somatic_variant_calling: ../somatic_variant_calling # REQUIRED | ||
| tools_ngs_mapping: [] # default to those configured for ngs_mapping | ||
| tools_somatic_variant_calling: [] # default to those configured for somatic_variant_calling | ||
| """ |
There was a problem hiding this comment.
The user must have control on the bed file for the regions considered for TMB. It must be part of the configuration.
| #number_indels=$(bcftools view -R {snakemake.input.bed_file} -v indels -H {snakemake.input.vcf}| wc -l) | ||
| TMB=`echo "1000000*($number_variants/$total_exom_length)" | bc -l ` | ||
| TMB_rounded=`printf "%.3f" $TMB` | ||
| to_json=$(cat <<EOF |
There was a problem hiding this comment.
You can simplify:
cat << EOF > {snakemake.output.json}
...
EOF
There was a problem hiding this comment.
Looks pretty good now.
There is a few more cosmetic changes to make black & flake8 happy. For the former, you should be able to make those changes automatically with the command black --line-length 100 ., provided that the version of black is the same that on the CI container.
As soon as all checks are passed I'll accept all changes.
| :raises UnsupportedActionException: if action not in class defined list of valid actions. | ||
| """ | ||
| if action not in self.actions: | ||
| actions_str = ", ".join(self.actions) |
There was a problem hiding this comment.
For future reference, you may want to consider self._validate_action(action)
No description provided.