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

Add error messages to MAE shell scripts #175

Merged
merged 3 commits into from
May 31, 2021
Merged

Add error messages to MAE shell scripts #175

merged 3 commits into from
May 31, 2021

Conversation

mumichae
Copy link
Collaborator

Whenever the output of GATK ASECounter is empty, DROP will continue with the empty files until DESeq2 throws an error.

One option is to throw an error every time the ASECounter output is empty. This is implemented here. There might be other possibilities in addressing this issue, so any discussion is welcome.

@mumichae mumichae requested a review from c-mertes February 21, 2021 09:09
Copy link
Contributor

@c-mertes c-mertes left a comment

Choose a reason for hiding this comment

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

The commits are fine just expend the error message for the user.

I just have one issue to raise here, but do not have the best solution yet. If you throw an error, the user can not finish the pipeline. Hence, he will not get the nice HTML reports before fixing those issues nor get a result file as it depends on all input of all samples. If you are happy with this for the end-user go ahead.

Maybe @vyepez88 has more throughs on this.

num_out=$(zcat "${output}" | wc -l )
if [ "${num_out}" -lt 2 ]
then
echo 'ERROR: No allele-specific counts'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a suggestion how to solve it.

  1. Checking the chromosomes (bam/vcf)
  2. Check the reference -> having 1 reference/fasta file per BAM could also solve dome of the issues here
  3. Removing the sample from MAE
  4. ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. This shouldn't happen with the new fixes
  2. Nick is implementing this feature

Comment on lines 57 to 61
echo "ERROR: no entries after filtering for SNVs"
echo "VCF ID: ${vcf_id}"
echo "VCF file: ${vcf_file}"
echo "BAM file: ${bam_file}"
exit 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above. Please add suggestions for the user to it.

  1. Do we do not have any heterozygous variants in the VCF file?
  2. Is the VCF file not correctly formated?
  3. Do we have issues with the chromosome style? (Not sure if this is really a problem in this script)
  4. ....

Copy link
Collaborator

@vyepez88 vyepez88 Feb 22, 2021

Choose a reason for hiding this comment

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

number 3. shouldn't be a problem in this script.
4. Check if vcf file exists and its index exists (although the pipeline might not even begin if this is not the case)

@vyepez88
Copy link
Collaborator

I'm fine with the users not getting the aggregated results table + html. They can anyway get the results of all the other samples by running the pipeline with the -k command. But in my experience, it either works for all files, or doesn't work at all.

@mumichae
Copy link
Collaborator Author

mumichae commented Mar 6, 2021

@c-mertes Would you suggest to change this error message to a warning then and deal with empty ouputs later in the pipeline?

Copy link
Contributor

@c-mertes c-mertes left a comment

Choose a reason for hiding this comment

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

looks all good now. thanks @mumichae

@c-mertes c-mertes merged commit b0e5c79 into dev May 31, 2021
@c-mertes c-mertes deleted the MAE_error_report branch May 31, 2021 11:28
c-mertes added a commit that referenced this pull request Jul 21, 2021
Major changes included in this update is:
* adapt to new snakemake version (#208)
* Speedup CI (use micromamba, #218)
* Add HTML cutoff for results (#217)
* Separate modules (#209)
* Update documentation (#228)

On top of this many small bug-fixes are included:
* #175 #192 #198 #231
* Simplify result reporting


Co-authored-by: Vicente Yepez <30469316+vyepez88@users.noreply.github.com>
Co-authored-by: Vicente <yepez@in.tum.de>
Co-authored-by: Michaela Mueller <51025211+mumichae@users.noreply.github.com>
Co-authored-by: Michaela Mueller <mumichae@in.tum.de>
Co-authored-by: Smith Nicholas <smith@in.tum.de>
Co-authored-by: nickhsmith <smithnickh@gmail.com>
Co-authored-by: Christian Mertes <mertes@in.tum.de>
nickhsmith added a commit that referenced this pull request Aug 11, 2021
* Update to version 1.1.0 (#229)

Major changes included in this update is:
* adapt to new snakemake version (#208)
* Speedup CI (use micromamba, #218)
* Add HTML cutoff for results (#217)
* Separate modules (#209)
* Update documentation (#228)

On top of this many small bug-fixes are included:
* #175 #192 #198 #231
* Simplify result reporting


Co-authored-by: Vicente Yepez <30469316+vyepez88@users.noreply.github.com>
Co-authored-by: Vicente <yepez@in.tum.de>
Co-authored-by: Michaela Mueller <51025211+mumichae@users.noreply.github.com>
Co-authored-by: Michaela Mueller <mumichae@in.tum.de>
Co-authored-by: Smith Nicholas <smith@in.tum.de>
Co-authored-by: nickhsmith <smithnickh@gmail.com>
Co-authored-by: Christian Mertes <mertes@in.tum.de>

* add splitting to MAE filterSNVs.sh

* fix broken link and maxVarFreqCohort

* fix broken link and maxVarFreqCohort

* Update test_MAE.py

* code review fixes

Co-authored-by: Michaela Mueller <51025211+mumichae@users.noreply.github.com>
Co-authored-by: Vicente Yepez <30469316+vyepez88@users.noreply.github.com>
Co-authored-by: Vicente <yepez@in.tum.de>
Co-authored-by: Michaela Mueller <mumichae@in.tum.de>
Co-authored-by: Smith Nicholas <smith@in.tum.de>
Co-authored-by: Christian Mertes <mertes@in.tum.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants