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

Patch release: Fix #449 (wrong contigs from aDNA workflow going to binning) #458

Merged
merged 7 commits into from
Jun 19, 2023
Merged

Patch release: Fix #449 (wrong contigs from aDNA workflow going to binning) #458

merged 7 commits into from
Jun 19, 2023

Conversation

maxibor
Copy link
Member

@maxibor maxibor commented Jun 14, 2023

Fixing #449

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/mag 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).

@github-actions
Copy link

github-actions bot commented Jun 14, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 9f86e7d

+| ✅ 155 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • nextflow_config - Config manifest.version should end in dev: '2.3.1'
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-06-19 09:30:27

jfy133
jfy133 reviewed Jun 14, 2023
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.

Conceptually 👍, test is failing though?

Minor requests:

  • please change the new input channel to the standard form, e.g.: contigs_ch -> ch_contigs_for_binrefinement
  • as this is a hotfix patch will also need a patch version bump (to 2.3.1)
  • also needs a changelog update

@skrakau @d4straub this is a very major bug for the aDNA functionality, so we want to do the patch release, which ideally needs another review. I will take responsibility for pulling the changes into dev once the release is made.

jfy133
jfy133 reviewed Jun 15, 2023
View reviewed changes
workflows/mag.nf Outdated Show resolved Hide resolved
d4straub
d4straub reviewed Jun 15, 2023
View reviewed changes
workflows/mag.nf Outdated Show resolved Hide resolved
@jfy133 @d4straub
Update workflows/mag.nf
9b619ad 
Co-authored-by: Daniel Straub <[email protected]>
@jfy133
Copy link
Member

jfy133 commented Jun 15, 2023

@maxibor just need version bump, changelog, and ✔️ from @d4straub and we are good to go

@d4straub
Copy link
Collaborator

version bump, changelog

agreed, will approve then

@maxibor maxibor linked an issue Jun 19, 2023 that may be closed by this pull request
Genome binning is performed on uncorrected contigs when selecting the ancient DNA subworkflow #449
Closed
@maxibor
Copy link
Member Author

maxibor commented Jun 19, 2023

@d4straub @jfy133 Version and changelog updated

jfy133
jfy133 reviewed Jun 19, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@jfy133 jfy133 self-requested a review June 19, 2023 09:11
d4straub
d4straub approved these changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub left a comment

Choose a reason for hiding this comment

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

LGTM!

@jfy133
Copy link
Member

jfy133 commented Jun 19, 2023

Thanks @maxibor ! If we get the ✔️ from Daniel, we can merge and I'll try and do the release today or tomorrow morning (before I have to travel)

@maxibor
Copy link
Member Author

maxibor commented Jun 19, 2023

BTW, this PR relies on #451 being merged before since the meta would differ otherwise

@maxibor
Copy link
Member Author

maxibor commented Jun 19, 2023

Changes from #451 were integrated in this PR

jfy133
jfy133 approved these changes Jun 19, 2023
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.

Thanks @maxibor ! This indeed makes sense in the patch (as agreed on slack) :)

@jfy133 jfy133 changed the title Fix #449 Fix #449 (wrong contigs from aDNA workflow going to binning) Jun 19, 2023
@jfy133 jfy133 changed the title Fix #449 (wrong contigs from aDNA workflow going to binning) Patch release: Fix #449 (wrong contigs from aDNA workflow going to binning) Jun 19, 2023
@jfy133 jfy133 merged commit 61deef8 into nf-core:master Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub approved these changes

@jfy133 jfy133 jfy133 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.

Genome binning is performed on uncorrected contigs when selecting the ancient DNA subworkflow
3 participants
@maxibor @jfy133 @d4straub