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 read group back and crumble for HiFi #68

Merged
merged 7 commits into from
Dec 6, 2023
Merged

Add read group back and crumble for HiFi #68

merged 7 commits into from
Dec 6, 2023

Conversation

priyanka-surana
Copy link
Contributor

No description provided.

@priyanka-surana priyanka-surana added bug Something isn't working enhancement Improvement of the existing features labels Jun 13, 2023
@priyanka-surana priyanka-surana added this to the 1.2.0 milestone Jun 13, 2023
@priyanka-surana priyanka-surana self-assigned this Jun 13, 2023
@priyanka-surana priyanka-surana requested a review from muffato June 28, 2023 14:24
Copy link
Member

@muffato muffato left a comment

Choose a reason for hiding this comment

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

Is there a small PacBio file we could use in the test profile ? Had to run the full test profile to see the outcome of this change.

Shane also suggested to add the -c and -p options to samtools merge in order to remove redundant @RG (and @PG) headers. Could you add that ?

conf/modules.config Outdated Show resolved Hide resolved
modules/nf-core/crumble/main.nf Outdated Show resolved Hide resolved
@priyanka-surana
Copy link
Contributor Author

priyanka-surana commented Jul 6, 2023

Is there a small PacBio file we could use in the test profile ? Had to run the full test profile to see the outcome of this change.

The PacBio sample in the test_full is one of the smallest. The only way I know to subset a PacBio BAM is to convert to a regular BAM. We can look into that more.

Shane also suggested to add the -c and -p options to samtools merge in order to remove redundant @RG (and @PG) headers. Could you add that ?

Done ✅

@priyanka-surana
Copy link
Contributor Author

priyanka-surana commented Jul 6, 2023

I updated the docker.registry which led to updating lots of modules. It is in prep for template update.

@priyanka-surana priyanka-surana requested a review from muffato July 6, 2023 13:07
Copy link
Member

@muffato muffato left a comment

Choose a reason for hiding this comment

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

My "full_test" run stopped because of a RUNLIMIT / exit code 140 on SANGERTOL_READMAPPING:READMAPPING:ALIGN_ONT:MINIMAP2_ALIGN (ilPolIcar4_T1).
errorStrategy in conf/base.config indeed doesn't contain the 140. Could you add that ? or make it match the nf-core template ? (which now has a range)

@priyanka-surana
Copy link
Contributor Author

My "full_test" run stopped because of a RUNLIMIT / exit code 140 on SANGERTOL_READMAPPING:READMAPPING:ALIGN_ONT:MINIMAP2_ALIGN (ilPolIcar4_T1).
errorStrategy in conf/base.config indeed doesn't contain the 140. Could you add that ? or make it match the nf-core template ? (which now has a range)

This might be because of the change in sanger.config because with the old version we had a default retry strategy. I have made the update, but will push later, since I was in the middle of some other changes too.

@muffato muffato marked this pull request as draft October 25, 2023 10:30
@muffato muffato marked this pull request as ready for review December 6, 2023 13:43
Copy link
Member

@muffato muffato left a comment

Choose a reason for hiding this comment

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

Approving as I have no concerns about the code, and it's been used in production for a few months !

@muffato muffato merged commit 0262a3f into dev Dec 6, 2023
@muffato muffato mentioned this pull request Dec 6, 2023
9 tasks
This was unlinked from issues Dec 8, 2023
@muffato muffato deleted the crumble branch February 28, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement Improvement of the existing features
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Crumble compression for all aligned files Add CRAM compression option
2 participants