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

Make the gene essentiality usage more user friendly #194

Merged
merged 21 commits into from
Sep 6, 2024
Merged

Make the gene essentiality usage more user friendly #194

merged 21 commits into from
Sep 6, 2024

Conversation

LaurenceKuhl
Copy link
Contributor

@LaurenceKuhl LaurenceKuhl commented Sep 5, 2024 •
edited
Loading

Hi!

I here modified the documentation and the way 4 modules (RRA, MLE, Drugz and Bagel2) are called in the workflow. The user now needs to only specify a contrast file and any of the modules (ie --rra, --mle, --drugz, --bagel2) to get the module running.

One of the modules was also not working properly (only running once) because I hadn't set the channels properly (with .first() at the end did the trick)

@github-actions GitHub Actions
Copy link

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

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit f46857f

+| ✅ 229 tests passed       |+
#| ❔   3 tests were ignored |#
!| ❗   4 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline
  • 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!

❔ Tests ignored:

  • files_exist - File is ignored: conf/test.config
  • files_exist - File is ignored: conf/test_full.config
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-09-06 12:13:48

jasmezz
jasmezz reviewed Sep 6, 2024
View reviewed changes
Copy link

@jasmezz jasmezz left a comment

Choose a reason for hiding this comment

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

Some minor comments on the formatting/typos. I am not familiar with the functionality itself, so if you want an opinion there, you should get an additional review from someone else.

docs/usage/screening.md Outdated Show resolved Hide resolved
docs/usage/screening.md Outdated Show resolved Hide resolved
docs/usage/screening.md Outdated Show resolved Hide resolved
docs/usage/screening.md Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
nextflow_schema.json Outdated Show resolved Hide resolved
workflows/crisprseq_screening.nf Outdated Show resolved Hide resolved
workflows/crisprseq_screening.nf Outdated Show resolved Hide resolved
workflows/crisprseq_screening.nf Outdated Show resolved Hide resolved
LaurenceKuhl and others added 9 commits September 6, 2024 13:59
@LaurenceKuhl @jasmezz
Update workflows/crisprseq_screening.nf
2c4b173 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update docs/usage/screening.md
6936128 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update docs/usage/screening.md
ff84f28 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update workflows/crisprseq_screening.nf
42e494c 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update docs/usage/screening.md
bda7258 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update docs/usage/screening.md
b8ca422 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update nextflow_schema.json
7ba7fee 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update nextflow_schema.json
16a4a57 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl @jasmezz
Update workflows/crisprseq_screening.nf
f46857f 
Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl
Copy link
Contributor Author

Hi @jasmezz I think regarding functionality it was more or less demanded to be like this by the users of the pipeline :)

jasmezz
jasmezz approved these changes Sep 6, 2024
View reviewed changes
Copy link

@jasmezz jasmezz left a comment

Choose a reason for hiding this comment

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

Ok then I'll approve from what I understand ;) Hint (if you want to accept my last docs suggestion), you can add [skip ci] to your commit message which prevents GHA from triggering the CI tests. Because it is only a tiny usage docs update, it is legitimate to bypass re-running all functional checks if you want.

docs/usage/screening.md Outdated Show resolved Hide resolved
@LaurenceKuhl @jasmezz
Update docs/usage/screening.md
59fa3d6 
[skip ci]

Co-authored-by: Jasmin Frangenberg <[email protected]>
@LaurenceKuhl LaurenceKuhl merged commit e9e9d4e into nf-core:dev Sep 6, 2024
mirpedrol
mirpedrol reviewed Sep 12, 2024
View reviewed changes
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Hi! sorry for reviewing a merged PR, but I am leaving 2 comments which should be fixed in another PR 🙂

workflows/crisprseq_screening.nf

if(params.mle && params.bagel2) {
ch_venndiagram = BAGEL2_PR.out.pr.join(MAGECK_MLE.out.gene_summary)
VENNDIAGRAM(ch_venndiagram)
Copy link
Member

Choose a reason for hiding this comment

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

The versions of VENNDIAGRAM are missing

modules/local/prepare_gpt_input.nf
path 'query.txt', emit: query

script:
template 'collect_gene_ids.py'
Copy link
Member

Choose a reason for hiding this comment

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

Where is this script?

modules/local/prepare_gpt_input.nf
Copy link
Member

Choose a reason for hiding this comment

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

I think this module is not used

@LaurenceKuhl LaurenceKuhl mentioned this pull request Sep 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirpedrol mirpedrol mirpedrol left review comments

@jasmezz jasmezz jasmezz approved these changes

@nvnieuwk nvnieuwk Awaiting requested review from nvnieuwk

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

Successfully merging this pull request may close these issues.
3 participants
@LaurenceKuhl @mirpedrol @jasmezz