Skip to content

Fix base probability calculation in generate_sequence to handle zero … - #1631

Closed
AgustinPardo wants to merge 1 commit into
nanoporetech:release-v2.1from
AgustinPardo:handle-zero-prob-in-generate-seq
Closed

AgustinPardo wants to merge 1 commit into
nanoporetech:release-v2.1from
AgustinPardo:handle-zero-prob-in-generate-seq

Conversation

@AgustinPardo

Copy link
Copy Markdown

Handle positions with zero accumulated probability when generating the decoded sequence and quality string

  • Avoid division by zero when totalProbs[i] is zero
  • Prevent log10(0) by clamping the error probability to the smallest positive float
  • Preserve the existing quality-score scaling and clamping behavior

@malton-ont

Copy link
Copy Markdown
Collaborator

Hi @AgustinPardo,

Thanks for your contribution.

Have you raised this because you've had an issue caused by this code (I don't see an open ticket)? I ask because this change introduces additional branching into a critical loop, which could have a deleterious effect on performance. As far as I can see it's not possible for the most probable base to have a base_probs of 0 and total_probs should sum to 1 so that can't be 0 either. Therefore this change seems to be redundant.

If you're experiencing something different, we'd like to take a look and solve the issue at source rather than masking it with this change.

@AgustinPardo

AgustinPardo commented Sep 9, 2026 •

Copy link
Copy Markdown
Author

Hi @malton-ont

Thanks for the quick review and the detailed explanation.

You're right , I raised this as a precautionary fix based on static code analysis rather than an observed failure in practice. I didn't have a concrete reproduction case, and I should have opened a ticket first to discuss whether the scenario is actually reachable before submitting the PR.

Your explanation makes sense: if totalProbs always sums to 1 by construction, then the guard is indeed redundant and the added branching in a hot loop is a valid concern.

@malton-ont malton-ont closed this Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants