Skip to content

Address PR #232 app comments: asymmetric placement tolerance + import cleanup (rebased onto 688d16a) #236

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.

Sign up for GitHub

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

Merged
hyperpolymath merged 1 commit into from
Oct 23, 2026
Merged

Address PR #232 app comments: asymmetric placement tolerance + import cleanup (rebased onto 688d16a) #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/analysis/clade_cumulus.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,15 @@ const DEFAULT_TAXONOMIC_RANKS = [
"Class", "Order", "Family", "Genus", "Species",
]

# Multi-candidate placements partition probability mass across kept edges and
# must sum to 1 within floating-point tolerance; single-candidate placements
# leave unplaced mass off the tree and require at least half the mass at the
# reported site (`PLACEMENT_SINGLE_MIN_LWR`).
const PLACEMENT_WEIGHT_SUM_TOL = 1e-5
# Placement tools truncate a query's candidate list at a likelihood-weight
# cutoff, so retained like-weight ratios can legitimately sum to a little
# under 1 (a single retained placement at 0.98 is complete evidence). A shortfall
# beyond the tolerance is a missing or discarded placement and stays undecided.
# A sum over 1 is never legitimate: weights are mutually exclusive placement
# ratios, so overweight evidence is malformed and is rejected by a tight upper
# bound while the lower shortfall is tolerated.
const PLACEMENT_WEIGHT_SUM_TOL = 0.05

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Separate parsing tolerance from unplaced-read accounting

Raising this shared constant to 0.05 also changes src/server/routes/clade_cumulus.jl:313-314, where it suppresses reporting any unplaced mass below 5%. For example, the existing route fixture has a valid placement with LWR 0.99; its missing 1% was previously returned as 0.05/0.10 unplaced reads, but now 0.99 < 0.95 is false, so both values become zero and the assertions at test/unit/test_clade_cumulus.jl:477-478 fail. Use a separate floating-point epsilon for unplaced-read accounting so the relaxed parser threshold does not hide discarded mass.

Useful? React with 👍 / 👎.

const PLACEMENT_WEIGHT_OVER_TOL = 1e-9
const PLACEMENT_SINGLE_MIN_LWR = 0.5

## Phylogenetic tree structures & Newick / jplace parsers
Expand Down Expand Up @@ -577,7 +581,7 @@ function parse_jplace(raw_json::AbstractString)::JplaceDocument
elseif length(cands) == 1 && cands[1].like_weight < PLACEMENT_SINGLE_MIN_LWR
push!(queries, QueryPlacement(q_ord, q_names, cands, :undecided,
"weight_sum_out_of_tolerance ($(round(weight_sum; digits=6)))", nothing))
elseif length(cands) > 1 && abs(weight_sum - 1.0) > PLACEMENT_WEIGHT_SUM_TOL
elseif length(cands) > 1 && (weight_sum > 1.0 + PLACEMENT_WEIGHT_OVER_TOL || weight_sum < 1.0 - PLACEMENT_WEIGHT_SUM_TOL)

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve missing mass in admitted short partitions

When this condition newly admits a partition such as 0.80 + 0.16, _graft_jplace_queries still computes each share as cand.like_weight / total_w at lines 1010-1022. That turns the weights into approximately 0.8333 + 0.1667 and allocates 100% of the ASV's reads to the grafted tips, manufacturing the missing 4% that the new comments describe as discarded or unplaced; before this change such a query never reached grafting because it was undecided. Preserve the raw candidate weights, or explicitly represent the missing share, so clade counts and frequencies do not disagree with the raw-LWR diversity calculation.

Useful? React with 👍 / 👎.

push!(queries, QueryPlacement(q_ord, q_names, cands, :undecided,
"weight_sum_out_of_tolerance ($(round(weight_sum; digits=6)))", nothing))
else
Expand Down
1 change: 0 additions & 1 deletion src/pipeline/phylogeny.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export REFERENCE_STEPS, PLACEMENT_STEPS, Step, placement_settings, run_workflow,
using SHA, JSON3, Dates, Logging, OrderedCollections
using ..PipelineLog
using ..Validation
import ..Tools
using ..Tools: tool_bin, _sq, _run_logged, _safe_optional_args, _num
import ..Tools
using ..RemoteExec: run_remote, checked_target
Expand Down
2 changes: 1 addition & 1 deletion test/unit/test_clade_cumulus.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ _cc_req(method::String, path::String, body::String = "") =
@test _single_at(CC.PLACEMENT_SINGLE_MIN_LWR).status === :valid
@test _single_at(0.49).status === :undecided
@test _partition(0.70, 0.30).status === :valid
@test _partition(0.80, 0.16).status === :undecided
@test _partition(0.80, 0.16).status === :valid
@test _partition(0.80, 0.24).status === :undecided
@test _partition(0.60, 0.60).status === :undecided

Expand Down
Loading