-
Notifications
You must be signed in to change notification settings - Fork 13
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
added background subtraction #48
base: dev
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.
Looks good to me! I would only change the exposure time to be a number to allow for floats as we have a dataset with such exposure times.
I'll be hopefully updating the nf-core module for Backsub soon so it adheres to nf-core standards more with conda support.
I'm leaning towards having a different step name than the tool name, so for the step name, I think it should be background/af_correction/channel_subtraction, with Backsub as the tool to do it. What are the preferences from your side?
@@ -34,6 +34,15 @@ | |||
"emission_wavelength": { | |||
"type": "integer", | |||
"errorMessage": "" | |||
}, | |||
"exposure": { | |||
"type": "integer" |
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.
"type": "integer" | |
"type": "number" |
From what I see in other nf-core pipelines, modules are imported and used under their original names (unless a module is used multiple times and must be aliased for uniqueness) and publishDir folder names also generally follow the module name. Sometimes publishDir paths are grouped under a higher level folder if there are many alternatives for the same step like we have for segmentation. The mcmicro-legacy module names and publishDir layout did not use this style, rather naming everything by its purpose, e.g "registration", "segmentation", "quantification", and we did start to use that same publishDir structure here in mcmicro 2.0. I think we should pivot to adopting the nf-core style for consistency though. |
@@ -19,6 +19,9 @@ params { | |||
// Illumination correction | |||
illumination = null | |||
|
|||
// Background subtraction | |||
backsub = false |
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 we need this, or can it be inferred by the presence of the relevant columns in the marker sheet?
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.
Oh, yes, that's a good idea. We could trigger background subtraction on the presence of a remove
column in the marker sheet.
Is there a better column to use?
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.
The column critical for the module would be background
, as only using the tool for channel removal seems like an unnecessary data duplication step.
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.
ok, sounds good, I'll use the background
column. Thanks!
I thought it might be used for something else as well, but it seemed like the remove
column would only be used for background subtraction.
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.
Just to add here, it might be good to check for both the presence of the background
and exposure
columns as both are required for channel subtraction. If you check by background
, and no exposure
column is given, the tool would give an error.
I think the best way to approach this would be to: check for presence of the background
column, if it is given, check the values in the column, if none are a string, we skip Backsub. Otherwise, check for presence of the exposure
column (and values), if not provided, an error should be thrown, and otherwise Backsub is run.
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.
Should validate that background
column values are a subset of the marker_name
values in validateInputMarkersheet
.
input[0] = Channel.of( | ||
[ | ||
[id:"TEST1", cycle_number:1, channel_count:4], | ||
"https://raw.githubusercontent.com/nf-core/test-datasets/modules/data/imaging/background_subtraction/cycif_tonsil_registered.ome.tif", |
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 don't think this is an actual background-subtraction test case, but structurally I guess it's fine.
channel_number + "," + cycle_number + "," + marker_name + "," + exposure + "," + background + "," + remove}] } | ||
.flatten() | ||
.map { it.replace('[]', '') } | ||
.collectFile(name: 'markers_backsub.csv', sort: false, newLine: true, storeDir: "${workDir}") |
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.
Don't use storeDir here. collectFile
returns a channel, and you should use that channel directly where needed downstream. Check out ch_mcquant_markers
and its usage in calling MCQUANT above.
.flatten() | ||
.map { it.replace('[]', '') } | ||
.collectFile(name: 'markers_backsub.csv', sort: false, newLine: true, storeDir: "${workDir}") | ||
BACKSUB(ASHLAR.out.tif, [[id:"$ASHLAR.out.tif[0]['id']"], "${workDir}/markers_backsub.csv"]) |
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.
As mentioned above, instead of an explicit path for the csv, use ch_backsub_markers
. You'll need to use combine
to join the collectFile-returned single-value channel with each entry in the meta-map channel for the second arg to BACKSUB.
I was under the impression our goal is to still keep the multiple choice aspect based on steps - especially when it will come to defining the "execution stream" - e.g. staging, registration, AF_correction, segmentation and quantification. The broader steps are defined and used to determine the "stream", but for each step (or several steps), we would allow for multiple options (e.g. staging, registration*, segmentation). I think it would be somewhat confusing to start mixing process names into the broader execution stream which is why I made the point above. |
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).nextflow run . -profile debug,test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).