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 the column db_type to database sheet #466

Merged
merged 18 commits into from
Jun 25, 2024
Merged

Add the column db_type to database sheet #466

merged 18 commits into from
Jun 25, 2024

Conversation

LilyAnderssonLee
Copy link
Contributor

Fix the issue #226

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/taxprofiler 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>).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,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).

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 11, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 2534c08

+| ✅ 261 tests passed       |+
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Only uncomment below if logic in toolCitationText/toolBibliographyText has been filled!
  • pipeline_todos - TODO string in ci.yml: You can customise CI pipeline run tests as required

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-06-25 06:45:50

@LilyAnderssonLee LilyAnderssonLee mentioned this pull request Apr 11, 2024
Closed
@LilyAnderssonLee LilyAnderssonLee removed the request for review from jfy133 April 11, 2024 11:32
@LilyAnderssonLee
Update ci.yml
abe18e5 
Update the mOTU database name in ci.yml
@LilyAnderssonLee
Update ci.yml
60087e7
jfy133
jfy133 reviewed Apr 16, 2024
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

I think this is veeery close now :D

I think once we have the optional column bit implemented I would start with testing :)

Otherwise this is looking very clean and concise :D!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
assets/schema_database.json Outdated Show resolved Hide resolved
subworkflows/local/profiling.nf Outdated Show resolved Hide resolved
subworkflows/local/profiling.nf Outdated Show resolved Hide resolved
LilyAnderssonLee and others added 5 commits April 17, 2024 08:04
@LilyAnderssonLee @jfy133
Update CHANGELOG.md
f8bd172 
Co-authored-by: James A. Fellows Yates <jfy133@gmail.com>
@LilyAnderssonLee
combine reads and db by db_type
1f5d5ea
@LilyAnderssonLee
Merge branch 'bouncy-basenji' into add_db_type
547a0aa
@LilyAnderssonLee
Update ci.yml
301119a 
correct the definition of db_type for both short and long reads
@LilyAnderssonLee
Update profiling.nf
749cd28
@LilyAnderssonLee LilyAnderssonLee mentioned this pull request Jun 17, 2024
Merged
@jfy133
Copy link
Member

jfy133 commented Jun 20, 2024 •
edited
Loading

OK I made some changes to make the channel manipulation more 'nextflowy', namely using transpose to 'duplciate' a channel but each of one of the two having a different short/long types separately in each

I then modify both database and read channels to have type as a 'meta-meta', to allow them to combine, and then move the type back into the meta after combining

@LilyAnderssonLee please test when you have time to make sure I didn't get something compeltely wrong

@LilyAnderssonLee
Copy link
Contributor Author

@jfy133 I have run the tests for the test, test_full, and test_krakenuniq, and all of them passed. Great job! I think we can merge this PR once all tests pass.

@jfy133 jfy133 self-requested a review June 25, 2024 06:53
@jfy133
Copy link
Member

jfy133 commented Jun 25, 2024

I forgot to check: did you check with an oldstyle samplesheet without the db_type column ? We wanted to make it optional and if so default to short;long right?

@LilyAnderssonLee LilyAnderssonLee merged commit 2a679b6 into nf-core:bouncy-basenji Jun 25, 2024
24 checks passed
@LilyAnderssonLee LilyAnderssonLee deleted the add_db_type branch June 25, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfy133 jfy133 jfy133 approved these changes

@sofstam sofstam Awaiting requested review from sofstam

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@LilyAnderssonLee @jfy133