-
Notifications
You must be signed in to change notification settings - Fork 1
[Do not merge!] Pseudo PR for first release #11
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
base: TEMPLATE
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first part of my review
Co-authored-by: Matthias Hörtenhuber <[email protected]>
Co-authored-by: Matthias Hörtenhuber <[email protected]>
Co-authored-by: Matthias Hörtenhuber <[email protected]>
Thanks @mashehu for this first review, I updated it accordingly Just one question regarding your other comment on the original PR: When doing the change you suggested about the containerOption, I now have this CI error again. It's because for cellpose (one specific segmentation tool, not always used) I need to have permission for its cache directory. I followed this nf-core docs to fix this (see cellpose conf here) Do you have a solution for it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
went through the rest.
- modules need to be split
- more documentation needs to be moved over to nf-core
nf-core pipelines lint
error need to be fixed- nextflow_schema.json is not valid (please use
nf-core schema build
to edit it in the future)
|
||
## Sopa parameters | ||
|
||
You'll also need to choose some Sopa parameters that you'll provide to Nextflow via the `-params-file` option. You can find existing Sopa parameter files [here](https://github.com/gustaveroussy/sopa/tree/main/workflow/config), and follow the [corresponding README instructions](https://github.com/gustaveroussy/sopa/blob/main/workflow/config/README.md) of to get your `-params-file` argument. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, but we having parameter files linked in an external repo is not really the nf-core way. we prefer to have everything nicely bundled together to avoid version mismatches and keep things reusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible to provide all the params directly in the command line, but since there are many parameters I just recommend some "presets" to get started, but it's not mandatory to use these
I think the main problem comes from the parameters nested hierarchy, which doesn't show up on the instruction section of sopa on the nf-core website, although they are documented on the nextflow schema
What's your suggestion?
def TRANSCRIPT_BASED_METHODS = ['proseg', 'baysor', 'comseg'] | ||
def STAINING_BASED_METHODS = ['stardist', 'cellpose'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you put this as enums in your nextflow_schema.json you can skip this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already list the possible options in the nextflow_schema, but here I want to check more complex logic, like here
I don't know how to do such check with a schema, do you know if it's possible?
assert params.read instanceof Map && params.read.containsKey('technology') : "Provide a 'read.technology' key" | ||
assert params.containsKey('segmentation') : "Provide a 'segmentation' section" | ||
|
||
// backward compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need this for a 1.0.0 release?
anyway you can also do deprecation with the nextflow_schema.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can be useful for Sopa users who were using Snakemake and want to move to nf-core/sopa
They could keep the same parameters and it should work, even though their parameters are one year old
Concerning the schema: is it also possible to handle the deprecation logic as in here? Again, I don't know how to update other parameters of the config directly based on the schema
Thanks @mashehu for the second round of review! I split the modules as requested and started adding more docs, but I still have a few questions (see my answer to your review above) Also, when running Regarding the schema, what’s the right command line to check that it’s valid? I tried |
sorry, looks like I linted the wrong version, all good there now concerning linting errors 👍🏻 |
Do not merge! This is a PR of dev compared to the TEMPLATE branch for whole-pipeline reviewing purposes. Changes should be made to dev and this PR should not be merged! The actual release PR is at