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

initial commit of kma_index #7236

Merged
merged 11 commits into from
Dec 20, 2024
Merged

initial commit of kma_index #7236

merged 11 commits into from
Dec 20, 2024

Conversation

Krannich479
Copy link
Contributor

@Krannich479 Krannich479 commented Dec 17, 2024

PR checklist

Closes #7235

  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the versions.yml file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda

@Krannich479 Krannich479 added the new module Adding a new module label Dec 17, 2024
@Krannich479 Krannich479 self-assigned this Dec 17, 2024
Copy link
Contributor

@famosab famosab left a comment

Choose a reason for hiding this comment

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

Looks good already :)

modules/nf-core/kma/index/main.nf Outdated Show resolved Hide resolved
modules/nf-core/kma/index/main.nf Outdated Show resolved Hide resolved
modules/nf-core/kma/index/meta.yml Outdated Show resolved Hide resolved
@Krannich479
Copy link
Contributor Author

Krannich479 commented Dec 19, 2024

Great catch @famosab ! I was never really satisfied with my solutions for output and prefix. After reading your comments, I first thought of creating four output channels, one for each file, because my style of [val(meta), "${meta.id}.kmaindex.*"] output might even violates the nf-core output format specification of one output channel per file (my 'db' output channel emits four files in my current state).
Then I looked into what bwa index does and I think it's a very elegant solution:

  1. def prefix takes the input file's baseName
  2. bwa index creates a subdirectory for all index files
  3. The module BWA_INDEX then emits only one channel ("index") which contains the subdirectory

I'd re-design the my KMA_INDEX module that way. What do you think?

@Krannich479 Krannich479 mentioned this pull request Dec 19, 2024
14 tasks
@SPPearce
Copy link
Contributor

Yes, I think that is the standard method to deal with index generation. The individual files on their own are useless, they are required in combination.

@Krannich479
Copy link
Contributor Author

kmaIndex_gitpod

Docker test succeeds in gitpod.

@Krannich479 Krannich479 requested a review from jfy133 December 20, 2024 09:15
@Krannich479
Copy link
Contributor Author

@jfy133 is this the kind of proof you proposed?

@Krannich479 Krannich479 requested a review from famosab December 20, 2024 09:34
@jfy133 jfy133 merged commit 3d107f4 into nf-core:master Dec 20, 2024
27 of 30 checks passed
@Krannich479 Krannich479 deleted the kma_index branch December 20, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new module: kma index
4 participants