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

Output bai when using ht mode #78

Closed
wants to merge 2 commits into from
Closed

Output bai when using ht mode #78

wants to merge 2 commits into from

Conversation

SPPearce
Copy link
Contributor

@SPPearce SPPearce commented Sep 18, 2024 •
edited
Loading

Fixing the issue noted in #74. The index is generated, but not currently published.
I also removed the .* at the start of the process selectors, because they have higher priority than without, which gives a subtle issue if people try to override without using that themselves.
I also updated the modules.config to use * to match multiple files where relevant.

@SPPearce SPPearce changed the base branch from master to dev September 18, 2024 06:07
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 18, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅

Posted for pipeline commit 409c5bc

+| ✅ 180 tests passed       |+
#| ❔   8 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-09-18 06:11:23

@nf-core nf-core deleted a comment from github-actions bot Sep 18, 2024
znorgaard
znorgaard requested changes Sep 19, 2024
View reviewed changes
Copy link
Collaborator

@znorgaard znorgaard left a comment

Choose a reason for hiding this comment

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

In general, we prefer being explicit about output files when reasonable to make it clear which files may be expected. With that in mind, we should stick to separate publish entries for bam and bai files.

conf/modules.config
@@ -56,30 +56,23 @@ process {
]
}

withName: '.*FASTQTOBAM' {
withName: 'FASTQTOBAM' {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe the .*FASTQTOBAM is so this can still be valid if fastquorum is used as a subworkflow/module in other contexts. We should leave the globs as is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the .*FASTQTOBAM is so this can still be valid if fastquorum is used as a subworkflow/module in other contexts. We should leave the globs as is for now.

I spent several hours a few months ago trying to debug why my config with 'ALIGN_RAW_BAM' wasn't overriding the current modules.config. It'll always match the name FASTQTOBAM, whether in a subworkflow or not, but this will also match any process including the string "FASTQTOBAM".
You should only use the .* if you want to match multiple processes.

@SPPearce
Copy link
Contributor Author

In general, we prefer being explicit about output files when reasonable to make it clear which files may be expected. With that in mind, we should stick to separate publish entries for bam and bai files.

Hmmm, I think that is needlessly complicated to be honest.

@znorgaard
Copy link
Collaborator

znorgaard commented Sep 20, 2024 •
edited
Loading

How about this #79?

Updated the process name specification to align with the nf-core rnaseq workflow use the simple name and simplified the bam/bai publishing while remaining explicit.

@SPPearce
Copy link
Contributor Author

Closing in favour of #79

@SPPearce SPPearce closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@znorgaard znorgaard znorgaard requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@SPPearce @znorgaard