Enhance LOCIDEX_MERGE
#42
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Notice: First implementation will break some of the tests. I will fix them shortly, basically the outputs look different now that LOCIDEX_MERGE is creating multiple different processes, each with it's own output file. |
apetkau
reviewed
Feb 13, 2025
There was a problem hiding this comment.
This looks amazing @sgsutcliffe. Thanks so much 😄 . I've added a few initial commments.
| @@ -0,0 +1,36 @@ | |||
| process LOCIDEX_CONCAT { | |||
| tag 'Concat LOCIDEX files' | |||
| label 'process_medium' | |||
There was a problem hiding this comment.
Could you switch to process_single? Unless you need the increased resources, not sure how much csvtk concat uses for large files.
There was a problem hiding this comment.
So csvtk has a --num-cpus with a default being 4. This led me to believe it can be speed up with multiple processers. I should add --num-cpus $task.cpus regardless. 8360c0d
nextflow.config
Outdated
| @@ -59,6 +59,9 @@ params { | |||
| gm_method = "average" | |||
| gm_delimiter = "." | |||
|
|
|||
| // Locidex | |||
| batch_size = 2 | |||
There was a problem hiding this comment.
I would default it to something much larger (maybe 1000?). Though we can adjust this based on results of our benchmarking.
For test cases we can have two tests with smaller batch sizes (to test the mv condition if one batch and csvtk if multiple).
|
Some by bifurcating the input into subchannels -- the output became inconsistent which led to flakey tests. This change 0d84be3 should make output consistent and reduce the flakey results of tests. |
|
Running a benchmark we have used on previous releases of |
|
The branch needed to be update -- didn't have the patch release on the branch. Was causing benchmark to fail |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
STRY0016853: Enhance
LOCIDEX_MERGEWe want to switch
LOCIDEX_MERGEsteps to use multiple processes ingasnomenclature, so that the process is more efficient and can handle larger volumes of data.Criteria
LOCIDEX_MERGE, allowing multiple instances to run in parallel.LOCIDEX_MERGEshould output CSV files of subsets of samples.gasnomenclatureto merge the sample subset CSV files into a single profiles file.Proposed Solution
Before running
LOCIDEX_MERGE, the list of MLST JSON files, that was orginally passed toLOCIDEX_MERGE, is subdivided into batches (size specified by the parameter--batch_size):Then downstream of
LOCIDEX_MERGEwe runcsvtk concaton the output (if more than one output file is generated). This performed by the moduleLOCIDEX_CONCATPR checklist
nf-core lint).nextflow run . -profile test,docker --outdir <OUTDIR>).docs/usage.mdis updated.CHANGELOG.mdis updated.README.mdis updated.