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

UMI-tools extract #71

Merged
merged 23 commits into from
Jul 24, 2024
Merged

UMI-tools extract #71

merged 23 commits into from
Jul 24, 2024

Conversation

emmarousseau
Copy link
Collaborator

Description

UMI-tools extract

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@emmarousseau emmarousseau requested a review from rcannood July 1, 2024 11:14
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Great job on reducing the test data size! This is so much better, thanks!

I have a small question:

src/umi_tools/umi_tools_extract/config.vsh.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

I'm a bit confused by some of the arguments in this component. I feel like maybe too much inspiration was drawn from the nf-core umitools/extract?

LMKWYT

src/umi_tools/umi_tools_extract/config.vsh.yaml Outdated Show resolved Hide resolved
Comment on lines 31 to 35
- name: --bc_pattern
type: string
description: |
The UMI barcode pattern to use e.g. 'NNNNNN' indicates that the first 6 nucleotides
of the read are from the UMI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this argument not required? Should there not be an alternative -p? What is the difference between -p PATTERN and --bc-pattern and --bc-pattern2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't notice that, it only appears in the help message but not in the documentation! I will add it, I believe it is an alternative for --bc-pattern only based on the output i get when using it.

Comment on lines 42 to 50
- name: --read1_out
type: file
required: true
description: Output file for read 1.
direction: output
- name: --read2_out
type: file
description: Output file for read 2.
direction: output
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently:

umi_tools extract:

  • Inputs:
    • R1: -I
    • R2: --read2-in
  • Outputs:
    • R1: -S
    • R2: --read2-out

This component:

  • Inputs:
    • R1: --input
    • R2: --read2_in
  • Outputs:
    • R1: --read1_out
    • R2: --read2_out

I propose changing the inputs of the component to something consistent. Options:

  • --input, --read2_in, --output, --read2_out
  • --input, --input_r2, --output, --output_r2

Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

LGTM!

@rcannood rcannood merged commit 3566232 into viash-hub:main Jul 24, 2024
2 checks passed
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.

2 participants