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

Major update of latest template from nf-core #262

Merged
merged 41 commits into from
Mar 13, 2023
Merged

Major update of latest template from nf-core #262

merged 41 commits into from
Mar 13, 2023

Conversation

ypriverol
Copy link
Member

@ypriverol ypriverol commented Mar 7, 2023

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

@ypriverol ypriverol requested a review from jpfeuffer March 7, 2023 14:49
@github-actions
Copy link

github-actions bot commented Mar 7, 2023

nf-core lint overall result: Passed ✅

Posted for pipeline commit 004872c

+| ✅ 155 tests passed       |+
#| ❔   3 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.7.2
  • Run at 2023-03-13 17:07:06

@jpfeuffer
Copy link
Collaborator

Most of this is done in nf-core#70

@jpfeuffer
Copy link
Collaborator

I like it better if we have one PR for template merge. And one PR for version update.

@WangHong007
Copy link
Contributor

@ypriverol @jpfeuffer In all DIA test, the identification and quantification work was skipped, and Dia-NN did not work, which caused pmultiqc not to receive the diann_report and mistook it for the DDA type.

@ypriverol
Copy link
Member Author

ypriverol commented Mar 8, 2023

We should run the identification and quant steps. Can you fix it @WangHong007

@jpfeuffer
Copy link
Collaborator

@ypriverol can you maybe do a new PR where you are just changing the version numbers. a) this will have a much cleaner commit history and b) we can be more sure about errors caused by this

@ypriverol
Copy link
Member Author

@jpfeuffer @WangHong007 @daichengxin are working already in the PR.

@jpfeuffer
Copy link
Collaborator

Ok. I just think you would save some time. Because most of the errors are not present on nf-core.

@jpfeuffer
Copy link
Collaborator

So I think the reason is the very complicated merge in this PR.

@daichengxin
Copy link
Collaborator

daichengxin commented Mar 8, 2023

Hi @jpfeuffer, All issues are solved except for TMT. I tested TMT locally, it also reported same problems. But I tested it using previous version, it's ok. These are two proteininference input files from before openms updating and 2.9.0. Could you review? Thanks a lot.
2.9.0
ID_mapper_merge.zip
before openms update
ID_mapper_merge.zip

@jpfeuffer
Copy link
Collaborator

jpfeuffer commented Mar 8, 2023

That was very helpful @daichengxin . We probably also need the idXMLs that were used in IDMapper. Do you have them?

@daichengxin
Copy link
Collaborator

daichengxin commented Mar 8, 2023

@timosachsenberg
Copy link

thanks I will look into it immediately

@timosachsenberg
Copy link

fixed in OpenMS/OpenMS#6758

Copy link
Collaborator

@jpfeuffer jpfeuffer left a comment

Choose a reason for hiding this comment

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

Pyopenms was not updated yet since bioconda is bugged.

CHANGELOG.md Outdated Show resolved Hide resolved
@ypriverol
Copy link
Member Author

@jpfeuffer we are ready. You can merge the PR.

Copy link
Collaborator

@jpfeuffer jpfeuffer left a comment

Choose a reason for hiding this comment

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

I hope merging is gonna be fine since it's a messy PR that somehow still shows the template changes. But other than that, looks good.

@jpfeuffer jpfeuffer merged commit 9e37892 into bigbio:dev Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants