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 limma for rnaseq #286

Merged
merged 50 commits into from
Oct 28, 2024
Merged

Add limma for rnaseq #286

merged 50 commits into from
Oct 28, 2024

Conversation

KamilMaliszArdigen
Copy link

@KamilMaliszArdigen KamilMaliszArdigen commented Sep 3, 2024 •
edited
Loading

This pR aims to add limma suport for rnaseq data with two modes pairwise and mixed model. Currently pairwise mode is integrated and mixed model is still in progress.

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 (nf-test test main.nf.test -profile test,docker).
  • 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).

@KamilMaliszArdigen KamilMaliszArdigen added the WIP This issue is already being worked on label Sep 3, 2024
@KamilMaliszArdigen KamilMaliszArdigen self-assigned this Sep 3, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 3, 2024 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅ ⚠️

Posted for pipeline commit d4fca39

+| ✅ 300 tests passed       |+
#| ❔   6 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • 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!
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.2
  • Run at 2024-10-28 13:40:54

@KamilMaliszArdigen KamilMaliszArdigen added enhancement New feature or request and removed WIP This issue is already being worked on labels Oct 11, 2024
pinin4fjords
pinin4fjords requested changes Oct 11, 2024
View reviewed changes
Copy link
Member

@pinin4fjords pinin4fjords left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I still think this should be done via the nf-core module you started work on at nf-core/modules#6753, see my comments there.

To re-iterate, I think a separate Limma Voom module is probably the way to go, and in the first instance it should implement the 'standard' Limma Voom pathway without additional custom code.

Also, you have Git problems to resolve. Actually this is probably because we need to do the template update. I'll try to get to that soon

@mzenczak
Copy link

@pinin4fjords could you give more details about your comment:
You appear to have broken the subway maps- labels are falling off the left hand side and you've lost the text in the file icons.
because I'm not quite sure what you meant :)

@pinin4fjords
Copy link
Member

Can you clarify which test profile you mean?

test_affy will be fine :-)

@pinin4fjords
Copy link
Member

@pinin4fjords could you give more details about your comment: You appear to have broken the subway maps- labels are falling off the left hand side and you've lost the text in the file icons. because I'm not quite sure what you meant :)

If you have a look at the PNG of the revised subway map, there are a number of problems:

  • 'Reference annotation' has fallen of the page at the top left
  • All the white labels on the file icons have dissappeared
  • The font in the key has changed to a serif font for some reason

maxulysse
maxulysse reviewed Oct 25, 2024
View reviewed changes
docs/images/workflow.png Outdated Show resolved Hide resolved
@KamilMaliszArdigen
Copy link
Author

KamilMaliszArdigen commented Oct 25, 2024 •
edited
Loading

Well regarding the pipeline map @pinin4fjords and @maxulysse. This is what is currently on dev and master:
https://github.com/nf-core/differentialabundance/blob/master/docs/images/workflow.svg - Only modification which we made is green line to limma for RNA Seq path.

If I understand correctly you expect that diagram to be improved. Can we create issue to adres the problem which is present in current version of the pipeline? I will take care of that during upcoming hackathon.

EDIT:

Well please ignore this one - I've used different tool to edit SVG than Inkscape and it all fall apart. It should be fixed now. (Still not sure why but in gtihub svg viewer the issues which you mentioned persist.

@KamilMaliszArdigen
Copy link
Author

Can you clarify which test profile you mean?

test_affy will be fine :-)

dev_report.zip
new_limma_report.zip

Limma results files are exactly the same and I didn't noticed any difference in html report.

@KamilMaliszArdigen
Copy link
Author

Well regarding the pipeline map @pinin4fjords and @maxulysse. This is what is currently on dev and master: https://github.com/nf-core/differentialabundance/blob/master/docs/images/workflow.svg - Only modification which we made is green line to limma for RNA Seq path.

If I understand correctly you expect that diagram to be improved. Can we create issue to adres the problem which is present in current version of the pipeline? I will take care of that during upcoming hackathon.

Well please ignore this one - I've used different tool to edit SVG than Inkscape and it all fall apart. It should be fixed now. (Still not sure why but in gtihub svg viewer the issues which you mentioned persist.

pinin4fjords
pinin4fjords reviewed Oct 27, 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 good- much simpler now, thank you. Last piece is to add something to the usage documentation.

@KamilMaliszArdigen
Copy link
Author

@pinin4fjords I've added small additions to point that limma is available now with rnaseq data. I didn't elaborated heavily as we don't do that in case of deseq2. I wanted to be consistent.

@pinin4fjords
Copy link
Member

@nf-core-bot fix linting

@KamilMaliszArdigen
Copy link
Author

@nf-core-bot fix linting

pinin4fjords
pinin4fjords reviewed Oct 28, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
KamilMaliszArdigen and others added 2 commits October 28, 2024 14:38
@KamilMaliszArdigen @pinin4fjords
Update README.md
854b81d 
Co-authored-by: Jonathan Manning <[email protected]>
@KamilMaliszArdigen @pinin4fjords
Update README.md
d4fca39 
Co-authored-by: Jonathan Manning <[email protected]>
pinin4fjords
pinin4fjords approved these changes Oct 28, 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.

Thanks- looking good now!

@KamilMaliszArdigen KamilMaliszArdigen merged commit 014cbe2 into dev Oct 28, 2024
16 checks passed
@KamilMaliszArdigen KamilMaliszArdigen deleted the add_limma_for_rnaseq branch October 28, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maxulysse maxulysse maxulysse left review comments

@pinin4fjords pinin4fjords pinin4fjords approved these changes

Assignees

@KamilMaliszArdigen KamilMaliszArdigen

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@KamilMaliszArdigen @pinin4fjords @mzenczak @maxulysse @nf-core-bot