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.

Sign up for GitHub

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

Jump to bottom

feat: replacing custom coverage QC scripts with "alfred qc" (#481) #482

Merged
merged 10 commits into from
Jan 22, 2024
Merged

feat: replacing custom coverage QC scripts with "alfred qc" (#481) #482

merged 10 commits into from
Jan 22, 2024

Conversation

holtgrewe
Copy link
Member

No description provided.

@holtgrewe holtgrewe linked an issue Jan 22, 2024 that may be closed by this pull request
Replace targeted coverage report with "alfred qc" #481
Closed
@holtgrewe holtgrewe force-pushed the 481-replace-targeted-coverage-report-with-alfred-qc branch from fe1aa02 to 6eaad9d Compare January 22, 2024 10:57
@holtgrewe
Copy link
Member Author

@Nicolai-vKuegelgen This will need:

  • rm -rf {work,output}/*/report/cov_qc

In ngs_mapping to ensure 100% reprocessing.

@coveralls
Copy link

coveralls commented Jan 22, 2024 •
edited
Loading

Coverage Status

coverage: 85.704% (-0.04%) from 85.739%
when pulling fdc174b on 481-replace-targeted-coverage-report-with-alfred-qc
into dc1d92b on main.

@holtgrewe holtgrewe enabled auto-merge (squash) January 22, 2024 16:15
Nicolai-vKuegelgen
Nicolai-vKuegelgen reviewed Jan 22, 2024
View reviewed changes
Copy link
Contributor

@Nicolai-vKuegelgen Nicolai-vKuegelgen left a comment

Choose a reason for hiding this comment

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

Only one thing I am not sure about, rest looks good.

snappy_wrappers/wrappers/varfish_annotator/bam_qc/wrapper.py
Comment on lines +40 to +42
for alfred_qc in snakemake.input.alfred_qc:
with gzip.open(alfred_qc, "rt") as inputf:
data = json.load(inputf)
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like only the last input file will be read into data, is that really intended?

Or should the next for-loop be indented one level further?

Copy link
Member Author

Choose a reason for hiding this comment

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

True

Nicolai-vKuegelgen
Nicolai-vKuegelgen approved these changes Jan 22, 2024
View reviewed changes
Copy link
Contributor

@Nicolai-vKuegelgen Nicolai-vKuegelgen left a comment

Choose a reason for hiding this comment

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

LGTM

@holtgrewe holtgrewe merged commit ae8fe98 into main Jan 22, 2024
5 of 7 checks passed
@holtgrewe holtgrewe deleted the 481-replace-targeted-coverage-report-with-alfred-qc branch January 22, 2024 16:29
@tedil tedil mentioned this pull request Jun 28, 2024
Merged
This was referenced Dec 9, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nicolai-vKuegelgen Nicolai-vKuegelgen Nicolai-vKuegelgen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace targeted coverage report with "alfred qc"
3 participants
@holtgrewe @coveralls @Nicolai-vKuegelgen