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

Merge branch 'dev' of github.com:nf-core/drugresponseeval into dev #11

Merged
merged 33 commits into from
Nov 25, 2024

Conversation

JudithBernett
Copy link
Collaborator

@JudithBernett JudithBernett commented Nov 15, 2024

  • Updated to the new template
  • Added tests that run with docker, singularity, apptainer, and conda
  • Added the docker container and the conda env.yml in the nextflow.config. We just need one container for all
    processes as this pipeline automates the PyPI package drevalpy.
  • Added usage and output documentation.
  • Fixed linting issues

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/drugresponseeval branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core pipelines lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • 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).

@JudithBernett JudithBernett marked this pull request as ready for review November 22, 2024 17:22
Copy link
Contributor

@mashehu mashehu left a comment

Choose a reason for hiding this comment

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

Some smaller things

.nf-core.yml Outdated Show resolved Hide resolved
.nf-core.yml Outdated Show resolved Hide resolved
CITATIONS.md Outdated Show resolved Hide resolved
README.md Outdated
@@ -36,16 +36,6 @@ DrEval catalog, you can increase your work's exposure, reusability, and transfer

# ![DrEval_pipeline](assets/DrEval_pipeline_simplified.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is the old pipeline name? also please use a more descriptive label/alt-text here (something like "pipeline diagram of nf-core/drugresponseeval"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks! Renamed it

.nf-core.yml Outdated Show resolved Hide resolved
// Singularity: Comment in for AMD64 build:
// process.container = 'oras://community.wave.seqera.io/library/pip_drevalpy:6ca244fae0c2fa29'
process.container = 'ghcr.io/daisybio/drevalpy:main'
process.conda = 'env.yml'

// Global default params, used in configs
params {
// For this pipeline
run_id = 'my_run'
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure about this param?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I just need one container or conda environment for all processes because they all depend on the same package. Is there a better place to set this?

Copy link
Member

Choose a reason for hiding this comment

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

The container and conda settings are fine.

Matthias is referring to the params.run_id. For example, you could use workflow.sessionId or workflow.runName here instead. I think the question is more, "Why is run_id a pipeline parameter?".

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, yes, thanks for clarifying this. yes, that's exactly what I meant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I see @mahesh-panchal! The idea was that people can, e.g., compare different models with each other in different runs, such that they get separate results for them. They might, e.g., just want to compare baseline models against one state-of-the-art model in one run but then, in another run, they want to do an ablation study of one state-of-the-art model and just compare those results against a naive baseline. Then those results would get saved into different subdirectories. Is there a more nextflow way to do this?

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 is generally fine then. It's better to have a user-defined informative name than an uninformative string of numbers/letters and/or unrelated names.

It's possible to do this all in one such that you supply inputs with an id, parameters, etc, and then use meta map to propagate this information so you can run it all at once, but that can be left as a future pipeline enhancement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll add it as an issue!

nextflow_schema.json Outdated Show resolved Hide resolved
JudithBernett and others added 6 commits November 25, 2024 10:39
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
@JudithBernett JudithBernett merged commit 3d00d07 into dev Nov 25, 2024
9 of 10 checks passed
@JudithBernett JudithBernett deleted the linting_and_testing branch November 25, 2024 11:19
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.

4 participants