Skip to content

funannotate train: operator-precedence bug in bamthreads calculation (aux_scripts/trinity.py) #1179

Description

@hyphaltip

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions