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

Some parameter changes, added qbic credits #254

Merged
merged 12 commits into from
Mar 22, 2024
Merged

Some parameter changes, added qbic credits #254

merged 12 commits into from
Mar 22, 2024

Conversation

WackerO
Copy link
Collaborator

@WackerO WackerO commented Mar 20, 2024

PR checklist

  • This comment contains a description of changes (with reason).
  • 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 pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/differentialabundance branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@WackerO
Copy link
Collaborator Author

WackerO commented Mar 20, 2024

For the future: Should there be two separate sets of params features_id/name_col? ATM, these params are used both for the counts file and the GTF; I think the two files should be treated separately, no?

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 20, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 0854d3f

+| ✅ 272 tests passed       |+
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.
  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-03-22 13:30:18

@WackerO WackerO mentioned this pull request Mar 20, 2024
Merged
10 tasks
@WackerO WackerO requested a review from pinin4fjords March 20, 2024 14:47
pinin4fjords
pinin4fjords requested changes Mar 21, 2024
View reviewed changes
Copy link
Member

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Nice work on the config ID tweaks, but I don't think moving options out of the respective sections is helpful for someone looking for those options.

README.md Outdated Show resolved Hide resolved
conf/modules.config Outdated Show resolved Hide resolved
conf/modules.config Show resolved Hide resolved
conf/modules.config Outdated Show resolved Hide resolved
conf/modules.config Outdated Show resolved Hide resolved
conf/test.config Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
@WackerO WackerO requested a review from pinin4fjords March 21, 2024 14:56
pinin4fjords
pinin4fjords approved these changes Mar 21, 2024
View reviewed changes
Copy link
Member

@pinin4fjords pinin4fjords left a comment

Choose a reason for hiding this comment

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

Looking pretty good, only a minor suggestion

nextflow_schema.json
@@ -119,7 +119,6 @@
},
"observations_name_col": {
"type": "string",
"default": "sample",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention in the description that we will now default to the same column as the ID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done!

@WackerO WackerO merged commit 2972621 into nf-core:dev Mar 22, 2024
12 checks passed
@WackerO WackerO deleted the simplify branch March 22, 2024 14:45
@WackerO WackerO mentioned this pull request Jul 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinin4fjords pinin4fjords pinin4fjords 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.
2 participants
@WackerO @pinin4fjords