Describe the bug
In runTrinityGG(), the number of samtools sort compression threads is
computed as:
https://github.com/nextgenusfs/funannotate/blob/target_1.9/rust_EVM_trinity_PASA/funannotate/aux_scripts/trinity.py#L54
bamthreads = (args.cpus + 2 // 2) // 2
Python operator precedence evaluates 2 // 2 first (= 1), so this is
actually (args.cpus + 1) // 2, not (args.cpus + 2) // 2 as the spacing
implies was intended (a "use half the CPUs, rounded up" comment would fit
(args.cpus + 1) // 2 for round-up-of-half, but the + 2 visually suggests
a different rounding intent that isn't what's executed).
Concretely:
Not a functional break either way, just not computing what the code visually
appears to intend, and worth fixing while touching nearby throughput-tuning
code.
Suggested fix
Either add explicit parens to compute the evidently-intended
(args.cpus + 2) // 2, or simplify to the clearer max(1, args.cpus // 2)
if that's the actual intent (leave headroom for the hisat2/pipe side of the
hisat2 | samtools sort pipeline). Whichever is chosen should be a
deliberate one-line fix, not a silent behavior change disguised as a
formatting cleanup.
What command did you issue?
funannotate train -i genome.fa --left_norm R1 --right_norm R2 --cpus N ...
(any invocation that reaches runTrinityGG()'s hisat2/samtools alignment
step)
OS/Install Information
Found via static code review; not tied to a specific OS.
Describe the bug
In
runTrinityGG(), the number ofsamtools sortcompression threads iscomputed as:
https://github.com/nextgenusfs/funannotate/blob/target_1.9/rust_EVM_trinity_PASA/funannotate/aux_scripts/trinity.py#L54
Python operator precedence evaluates
2 // 2first (= 1), so this isactually
(args.cpus + 1) // 2, not(args.cpus + 2) // 2as the spacingimplies was intended (a "use half the CPUs, rounded up" comment would fit
(args.cpus + 1) // 2for round-up-of-half, but the+ 2visually suggestsa different rounding intent that isn't what's executed).
Concretely:
args.cpus=2: intended-looking(2+2)//2=2, actual(2+1)//2=1— happensto be harmless here since 1 is a reasonable thread count at cpus=2, which
is likely why this has gone unnoticed.
args.cpus=8:(8+2)//2=5vs actual(8+1)//2=4— diverges once CPUcounts increase (e.g. as part of fixing funannotate train: Trinity-GG Butterfly assembly pool sized cpus-1, wastes requested parallelism (serial at --cpus 2) #1178, which will make higher
--cpusvalues more effective for this stage and more likely to be usedin practice).
Not a functional break either way, just not computing what the code visually
appears to intend, and worth fixing while touching nearby throughput-tuning
code.
Suggested fix
Either add explicit parens to compute the evidently-intended
(args.cpus + 2) // 2, or simplify to the clearermax(1, args.cpus // 2)if that's the actual intent (leave headroom for the hisat2/pipe side of the
hisat2 | samtools sortpipeline). Whichever is chosen should be adeliberate one-line fix, not a silent behavior change disguised as a
formatting cleanup.
What command did you issue?
funannotate train -i genome.fa --left_norm R1 --right_norm R2 --cpus N ...(any invocation that reaches
runTrinityGG()'s hisat2/samtools alignmentstep)
OS/Install Information
Found via static code review; not tied to a specific OS.