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

Add umicollapse as an alternative to umi-tools #1369

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Add umicollapse as an alternative to umi-tools #1369

wants to merge 9 commits into from

Conversation

siddharthab
Copy link

@siddharthab siddharthab commented Sep 3, 2024 •
edited
Loading

Checklist:

@siddharthab
Add umicollapse as an alternative to umi-tools
fc13650 
TODO:
- [ ] Benchmark paired ends mode
- [ ] Include in multiqc
@siddharthab siddharthab mentioned this pull request Sep 3, 2024
Open
MatthiasZepper
MatthiasZepper approved these changes Sep 4, 2024
View reviewed changes
Copy link
Member

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

Excellent work, clean and well-structured! I think, you found very smart solutions to not duplicate too much of the code.

Of course, we should wait for the lead developers of the pipeline to review as well prior to merging, but in general you have my support for this addition! Ideally, your fix for umicollapse would also be included in the form of an updated tool version.

Please don't forget the Changelog later and also to add your name to the contributors (well deserved)!

nextflow_schema.json Outdated Show resolved Hide resolved
nextflow.config
@@ -30,6 +30,7 @@ params {
with_umi = false
skip_umi_extract = false
umitools_extract_method = 'string'
umi_dedup_tool = 'umicollapse'
umitools_grouping_method = 'directional'
Copy link
Member

Choose a reason for hiding this comment

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

Reading that makes we wonder if we should be conservative regarding the parameter names or make them more generic, e.g. renaming umitools_grouping_method to something like umi_grouping_method / umi_dedup_grouping_method etc.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I thought about that too. Depends on your policy for introducing breaking changes between versions. If the authors are OK, I can rename these parameters to be more generic.

workflows/rnaseq/main.nf Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/nextflow.config Show resolved Hide resolved
workflows/rnaseq/nextflow.config Show resolved Hide resolved
siddharthab
siddharthab commented Sep 4, 2024
View reviewed changes
Copy link
Author

@siddharthab siddharthab left a comment

Choose a reason for hiding this comment

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

Thanks for the quick review. I am tracking all remaining items in the PR summary.

nextflow.config
@@ -30,6 +30,7 @@ params {
with_umi = false
skip_umi_extract = false
umitools_extract_method = 'string'
umi_dedup_tool = 'umicollapse'
umitools_grouping_method = 'directional'
Copy link
Author

Choose a reason for hiding this comment

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

Yes, I thought about that too. Depends on your policy for introducing breaking changes between versions. If the authors are OK, I can rename these parameters to be more generic.

nextflow_schema.json Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Show resolved Hide resolved
workflows/rnaseq/nextflow.config Show resolved Hide resolved
@siddharthab
Keep umitools as the default
761c56d 
Signed-off-by: Siddhartha Bagaria <[email protected]>
@siddharthab siddharthab marked this pull request as ready for review September 4, 2024 16:46
@siddharthab siddharthab mentioned this pull request Sep 4, 2024
Open
@siddharthab
Copy link
Author

I checked the output of UMICollapse, and it definitely does not follow the assumptions stated in prepare_for_rsem.py. So PREPARE_FOR_RSEM will be needed for UMICollapse as long as it is needed for umi-tools.

@MatthiasZepper MatthiasZepper mentioned this pull request Oct 2, 2024
Merged
11 tasks
@siddharthab
Copy link
Author

@MatthiasZepper all remaining tasks are now done. Please take another look.

@MatthiasZepper
Copy link
Member

@MatthiasZepper all remaining tasks are now done. Please take another look.

Unfortunately, the UMICollapse module has not yet been upgraded...

@MatthiasZepper
Copy link
Member

@siddharthab: The module is finally upgraded, so you can replace the local path with the up-to-date module.

@siddharthab
Copy link
Author

@siddharthab: The module is finally upgraded, so you can replace the local path with the up-to-date module.

Thanks. Can you check if everything looks good now? I just ran nf-core modules update umicollapse, but do not know if I need to do more.

pinin4fjords
pinin4fjords reviewed Nov 6, 2024
View reviewed changes
Copy link
Member

@pinin4fjords pinin4fjords left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can we uppercase the new process aliases please? My tiny little mind will get confused otherwise.

workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
workflows/rnaseq/main.nf Outdated Show resolved Hide resolved
@siddharthab @pinin4fjords
Apply suggestions from code review
05513ee 
Co-authored-by: Jonathan Manning <[email protected]>
@siddharthab
Copy link
Author

Can we uppercase the new process aliases please?

@pinin4fjords, done.

MatthiasZepper
MatthiasZepper approved these changes Nov 12, 2024
View reviewed changes
Copy link
Member

@MatthiasZepper MatthiasZepper left a comment

Choose a reason for hiding this comment

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

I can only reiterate that I would like to see this merged. In my opinion, an outstanding job.

@pinin4fjords
Copy link
Member

@nf-core-bot fix linting

@pinin4fjords
Copy link
Member

Sorry, good in principle now for me (maybe some factoring out we can do later). Just need to get tests to run + pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinin4fjords pinin4fjords pinin4fjords left review comments

@MatthiasZepper MatthiasZepper MatthiasZepper 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.
4 participants
@siddharthab @MatthiasZepper @pinin4fjords @nf-core-bot