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

add MSstatsTMT and update MSstats #158

Merged
merged 17 commits into from
Apr 29, 2022
Merged

add MSstatsTMT and update MSstats #158

merged 17 commits into from
Apr 29, 2022

Conversation

daichengxin
Copy link
Collaborator

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

@github-actions
Copy link

github-actions bot commented Apr 22, 2022

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 91f0ac9

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

❗ Test warnings:

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.3.2
  • Run at 2022-04-26 08:52:28

nextflow.config Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
nextflow.config Outdated Show resolved Hide resolved
@ypriverol
Copy link
Member

@daichengxin MSstats is failing with DIANN

@daichengxin
Copy link
Collaborator Author

ok. I am testing locally.

@ypriverol
Copy link
Member

I think is because in the last tests we have included only two files in the DIANN with no multiple conditions. But this is only a guess.

@daichengxin
Copy link
Collaborator Author

Yes. you are right. I use two conditions locally to test. Need to update the test data

@daichengxin
Copy link
Collaborator Author

The test passed. Need to upload the latest dia test data to nfcore, then recover test_dia config. https://raw.githubusercontent.com/daichengxin/quantms/dev/assets/PXD026600.sdrf.tsv

@daichengxin daichengxin linked an issue Apr 26, 2022 that may be closed by this pull request
@jpfeuffer
Copy link
Collaborator

Why are you not using the newest version with argument parsing? Because of the additional dependency?
https://github.com/nf-core/proteomicslfq/blob/dev/bin/msstats_plfq.R

@daichengxin
Copy link
Collaborator Author

Yes. argparse is not in the container

bin/msstats_plfq.R Outdated Show resolved Hide resolved
Co-authored-by: Julianus Pfeuffer <[email protected]>
bin/msstats_tmt.R Outdated Show resolved Hide resolved
Co-authored-by: Julianus Pfeuffer <[email protected]>
@jpfeuffer
Copy link
Collaborator

Can we maybe have the logging changes separate? I don't think this is ready for release.

@jpfeuffer
Copy link
Collaborator

jpfeuffer commented Apr 26, 2022

E.g. it needs a lot of testing, the testdata needs to be on nf-core, I think it is better to only have one MSstats step/script (we need to check whether there are more parameters that we can combine).

@daichengxin
Copy link
Collaborator Author

So do we need to remove the logging changes in this PR?

@jpfeuffer
Copy link
Collaborator

No but create a PR with just the logging changes.

@jpfeuffer
Copy link
Collaborator

Use git cherry-pick on a new branch

@ypriverol
Copy link
Member

Can I merge this PR @jpfeuffer ?

@jpfeuffer
Copy link
Collaborator

No, see my comments

@ypriverol
Copy link
Member

We need two different PRs? Why we can combine both changes here if they are correct.

@jpfeuffer
Copy link
Collaborator

because then we have to wait until everything else is correct. MSstats is not a priority. A working pipeline is.

@ypriverol
Copy link
Member

But @daichengxin changed the MSstats as requested, then both things are OK now.

@jpfeuffer
Copy link
Collaborator

jpfeuffer commented Apr 27, 2022

E.g. it needs a lot of testing, the testdata needs to be on nf-core, I think it is better to only have one MSstats step/script (we need to check whether there are more parameters that we can combine).

We need more param combinations to be tested. Plus the rest of the comment.

@ypriverol
Copy link
Member

We need two things here:

  • load the testdata into nf-core, this is mainly the SDRF of the DIANN tests.
  • increase the test coverage for MSstats.

Can we, merge this PR and do another one replacing the SDRF and loading into NF-core and add an issue in the future for more testing? I just want to be able to contribute now.

@jpfeuffer
Copy link
Collaborator

You can contribute to this PR. I just want to merge into master soon, and do not want to have half-ready PRs in there.

@ypriverol ypriverol mentioned this pull request Apr 28, 2022
10 tasks
@daichengxin
Copy link
Collaborator Author

I got the changes that needs to be done. But I'm afraid I won't be able to do it soon because I'm not very familiar with the mechanics of CI.

@jpfeuffer jpfeuffer merged commit ae9c934 into bigbio:dev Apr 29, 2022
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.

Reevaluate logging
3 participants