Ph/pr50 retold by hansenp · Pull Request #52 · P2GX/BOQA

hansenp · 2026-09-02T13:16:44Z

Sealed BOQA-blended refactor, in reviewable commits

This is the code of #50, unchanged, split into eleven commits with messages that
say what each one does. Nothing is fixed, added or left out: the tree at the tip
of this branch is byte-identical to sealed. The goal is to make #50 reviewable,
and to record what reading it turned up.

It is stacked on #51, which does the same for #47, because #50 is based on
bboqa. If #50 is merged instead, this PR can be closed — the value here is the
commit messages and the findings below, not the code.

sealed moved while this branch was being written. #50 had four commits when I
started and six when I finished; b17bae9 and 6e53bda arrived in between, the
second on the morning of the meeting. Commits 1–9 here retell the first four,
commits 10 and 11 the two that followed, so the boundary stays visible.

What the change does

One decision drives the whole PR:

Disease annotations stop being looked up inside boqa-core and start being
supplied by the caller. TargetDisease carries its own
Set<TermId> observedHpoIds.

Everything else follows from it:

TargetDisease becomes a sealed interface, PhenotypeOnly / PhenotypeAndGene,
both carrying their HPO terms.
BlendedCounter keeps only the ontology. Counter.computeBoqaCountsFromDisease
takes two term sets and counts one against the other, with no notion of which
disease it is looking at; BoqaCountsNew therefore drops the disease id and label.
CandidateDiseaseNew builds the candidate list from target diseases alone, with
no HpoDiseases lookup. A blend is its set of components; the melded
OMIM:a-OMIM:b disease is gone.
BoqaPatientAnalyzerNew takes a patient, a counter and a candidate list and does
the whole analysis — raw log scores, softmax normalization, conversion to
CandidateResult, the improvement filter, sorting, limiting.
BoqaBlendedExomiserAnalyser shrinks from a forty-line scoring loop to a
four-line delegation.

Also included: a PlantUML class-diagram plugin bound to the compile phase.

What this changes relative to #47

#47's question 1 is answered "yes, and further". I asked on #51 whether the
blended path should stop going through DiseaseData. #47 had BlendedCounter
reading HpoDiseases directly; #50 removes even that. Core now holds no
disease-annotation source on the blended path at all.

The counter bug from #51 is fixed. #47's BlendedCounter stored raw
presentAnnotations() as its disease layer while expanding the patient side, so a
perfect match scored hundreds of false positives. computeBoqaCountsFromDisease
expands both sides. Scoring a disease against its own annotations:

OMIM:620851 (ReNU syndrome), 198 direct annotations
#47 BlendedCounter : tp=198  fp=335  tn=2100  fn=0   log(P) = -3757.9
#50 BlendedCounter : tp=533  fp=0    tn=2100  fn=0   log(P) = -1227.4
BoqaSetCounter     : tp=533  fp=0    tn=2100  fn=0   log(P) = -1227.4

Exact agreement with plain BOQA on that pair.

The analysis leftovers from #45 are deleted. analysis.TargetDisease and
analysis.BlendedResult are gone, answering #51's question 5.

The scoring moves into core. #47 scored inside the Exomiser wrapper; #50 puts
it in BoqaPatientAnalyzerNew, which is the shape the comment in the wrapper says
was agreed.

Not changed from #47: gene-disjointness still compares gene symbols while the
CLI compares gene IDs; alpha and beta are still fixed to defaultParams();
the improvement filter is still a bare > rather than a Bayes factor. #51's
questions 2, 3 and 4 are all still open.

Findings

Review notes; none are fixed on this branch.

The blended path cannot run. Two independent crashes, both verified against
the compiled classes at this tip.

CandidateDiseaseNew.createCandidateDiseases builds its list from
createSingleDiseaseCandidates, which ends in Stream.toList() — unmodifiable —
then calls .add() on it (CandidateDiseaseNew.java:92). Calling the wrapper the
way Exomiser would, with two gene-disjoint anchors:

two gene-disjoint anchors -> THREW java.lang.UnsupportedOperationException
   at CandidateDiseaseNew.lambda$createCandidateDiseases$1(CandidateDiseaseNew.java:92)

It is masked only when there is nothing to blend: fewer than two anchors, or every
pair sharing a gene.

Behind it, BoqaPatientAnalyzerNew.computeBoqaResults sorts the list returned by
computeBoqaResultsRawLog, also from Stream.toList()
(BoqaPatientAnalyzerNew.java:121). Handing it a candidate list built by hand:

computeBoqaResultsRawLog -> 2 results, first score -34.54370392354653
computeBoqaResults       -> THREW java.lang.UnsupportedOperationException
   at BoqaPatientAnalyzerNew.computeBoqaResults(BoqaPatientAnalyzerNew.java:121)

Fixing either one still leaves the other. The second is worth fixing by deletion:
everything after that sort is order-independent — maxLogP comes from .max(),
the sum from a full reduce, and the method ends with .sorted().limit(...) on the
CandidateResults — so removing the line fixes the crash and changes no output.

Set.of(a, b) in makeAllowedCombinations rejects duplicates, so the same disease
anchored twice throws IllegalArgumentException: duplicate element rather than
being skipped the way a same-gene pair is.

The test suite cannot run. ./mvnw test fails at testCompile, so zero of the
~3266 tests execute. BlendedResultTest is a live test of analysis.BlendedResult,
which commit 3 deletes without touching the test; today's commit renames the file to
BlendedDiseaseNewDiseaseResultResultTest.java rather than deleting it. Nothing
covers any of the new code. Note that mvn test-compile reports BUILD SUCCESS on a
warm target/ — Maven does not recompile an untouched test when a main class
disappears — so this is invisible locally until clean. No CI runs on sealed
(gh pr checks 50: no checks reported), so nothing caught it.

Counter no longer means anything. BlendedCounter.computeBoqaCounts returns a
hard-coded constant and getDiseaseIds() returns an empty set:

stub computeBoqaCounts -> BoqaCounts[diseaseId=, diseaseLabel=, tp=1, fp=1, tn=1, fn=1]
stub getDiseaseIds     -> []

while the interface default of computeBoqaCountsFromDisease throws
RuntimeException("... needs refactor"), which is what BoqaSetCounter inherits.
Neither implementation satisfies the interface: one lies, the other throws. A caller
holding a Counter gets plausible-looking garbage rather than an error.

Blended results lost their counts. Today's commit removes counts() from
CandidateResult and the field from BlendedResult, so toCandidateResult
computes the blend's counts and discards them, flagged with
// TODO actually make sure BlendedResults have also counts and score. The loss is
asymmetric: a SingleResult still reaches its counts through its DiseaseComponent.
Since a blend's counts are the only evidence separating "this blend explains more
phenotype" from "this blend has a bigger layer", this removes the reason to trust
the ranking. #45's BlendedResult carried componentCounts and blendedCounts for
exactly this.

Normalization spans singles and blends. The softmax runs over the whole
candidate list, so with n anchors the denominator has n + n(n−1)/2 terms. Single
diseases are diluted by the presence of blends, which makes these scores
incomparable with plain BOQA over the same anchors; and because the improvement
filter runs after normalization, what is returned does not sum to 1 either.

The HP:0000118 filter is still missing. isPhenotypicFeature is written into
BlendedCounter and never called, so nothing restricts a caller's terms to
phenotypic abnormalities the way BoqaSetCounter does on ingest. With annotations
now supplied by the caller, this matters more than in #47, not less. The agreement
in the table above holds because ReNU's annotations happen to be all phenotypic
abnormalities.

Guards removed alongside their tests. CandidateResult.Blended's
"at least two components" check is commented out, not deleted, in the same PR that
deletes the only test of that invariant. An empty blend is now constructible, and
naiveImprovement ends .max().orElse(0.0): with raw log scores that sentinel
means "reject", with normalized probabilities it means "accept". The same line
changes meaning between commits 7 and 8.

Four parallel type families, and dead code being maintained.
BoqaCounts/BoqaCountsNew, CandidateDisease/CandidateDiseaseNew,
BoqaResult/BoqaResultNew, BoqaPatientAnalyzer/BoqaPatientAnalyzerNew, with
no removal path stated. CandidateDisease has been unreferenced since commit 9 —
an unused import in BlendedCounter and a stale comment are all that name it — yet
today's commit renamed it throughout, ~43 lines of edits to a class nothing
constructs. CandidateDiseaseNew's logger is declared as
getLogger("org.p2gx.boqa.core.diseases.CandidateDisease"), so it logs under the
dead class's name.

Smaller things. The wrapper's constructor still runs DiseaseDataPhenolIngest.of
over all diseases into a DiseaseData field nothing reads — worse here than in #47,
since the design now says annotations come from the caller. resultsLimit is
hard-coded to 100000 with a TODO. Util.reScaledRawLogBoqaExomiserScoresNew and
computeBoqaResultsRescaled are dead, and the rescale still gives the best
candidate 1.0 however poorly it fits and yields NaN when scores are equal.
BoqaCountsNew's guard rejects negatives with the message "must be greater than
zero". BoqaPatientAnalyzerNew's javadoc promises a BoqaAnalysisResult and points
at BoqaSetCounter, neither of which it uses. Renames left dangling references: the
initLayer examples in OntologyTraverser's javadoc, and
DiseaseDataCmpParsePhenolIngestTest in DiseaseDataParser's. The PlantUML plugin
is bound to compile, so it runs on every build and writes a 17.9 KB generated file
into the repo root that is neither tracked nor ignored; a clean checkout now needs
network for its first build.

The naming is not settled. These types have been renamed three times in three
weeks: TargetDisease.Disease/Gene → Phenotype/Gene →
PhenotypeOnly/PhenotypeAndGene; the disease-data tests twice, ending at
PhenotypeOnlyPhenotypeAndGeneAssociationsTest — a class testing gene-to-disease
associations — and BlendedDiseaseNewDiseaseResultResultTest, which has "Result"
twice and still tests a deleted class. PhenotypeAndGene is a genuine improvement;
SingleDiseaseNew is not.

How this branch was built

Each commit was produced by checking the relevant paths out of sealed at the
commit being retold, then verified: all eleven compile main sources on their own,
and git diff 6e53bda at the tip is empty.

Test sources compile through commit 2 and stop at commit 3, and never recover. That
matches the endpoint; it is not an artifact of the split.

Three commits pass through states that existed on no machine, because #50 delivers
them as blobs. Commit 1 updates the initLayer call site in BlendedCounter that
commit 5 then deletes. Commit 4 updates two record-deconstruction patterns and one
factory call in the wrapper, replaced in commit 9. Commit 7 adds a four-line
asCountsNew bridge in the wrapper, also removed in commit 9. The endpoint is
unaffected.

Questions

Should BlendedResult get its counts back before this merges? Without them a
blend's ranking cannot be checked by a reader, and the asymmetry with
SingleResult means the two variants report different things.
Should normalization run over single diseases only, so blended scores stay
comparable with plain BOQA — or is a distribution over the joint candidate space
what is wanted, given the candidates are not mutually exclusive?
Is dropping the melded disease identity intentional? Prototype class for Blended BOQA Exomiser integration #43's TODO 2 asked for
OMIM:a-OMIM:b, Blended BOQA Exomiser integration #47 built it, this removes it. If Exomiser maps components back
onto its own per-gene results it may not need one — but components is a Set,
so a caller rebuilding a label gets no stable order.
Whose job is the HP:0000118 filter now that the caller supplies annotations,
and should TargetDisease say so?
Is one Counter interface with two half-implementations the intended end state?
If it is scaffolding, UnsupportedOperationException on both sides would be
honest where a hard-coded BoqaCounts is not.
What is the removal plan for the *New pairs? A reviewer cannot currently tell
which of each pair to build on, and dead code is being kept in sync by hand.
Should the PlantUML plugin move to a profile writing into target/, and should
it be its own PR? It is the only commit here that touches no Java.
Still open from Ph/pr47 retold #51: gene symbols or gene IDs for disjointness, and should the
CLI and this path share the rule? Should the improvement filter be a Bayes factor
threshold, and optional? Should alpha and beta stay fixed in the wrapper?

OntologyTraverser.initLayer named the BOQA concept it served rather than the operation it performs: expanding a set of HPO terms with their ancestors. The new name says what the method returns, which matters because the same call expands both the patient layer and the disease layer. Renames the method and its three call sites in BoqaSetCounter and BlendedCounter, and the corresponding test method. No behavior change. Two usage examples in the class javadoc still name initLayer; they are left as they are on the source branch.

Renames five test classes: DiseaseDataParseIngestTest, DiseaseDataPhenolIngestTest, DiseaseDataCmpParsePhenolIngestTest, BlendedDiseaseDataTest and DiseaseGeneAssociationsTest, each to a Phenotype* name, together with their self-references. The names anticipate renaming DiseaseData to PhenotypeData, which holds phenotype annotations keyed by disease rather than diseases. That rename does not happen here, so the tests name classes that do not exist yet. No test is added, removed or changed otherwise. A javadoc reference in DiseaseDataParser still names the old test and is left as it is on the source branch.

These two records were the Exomiser-facing input and output types introduced in #45. #47 superseded them with diseases.TargetDisease and CandidateResult but left the files in place, unreferenced by any main source. Deleting them leaves BlendedResultTest referring to classes that no longer exist, so test sources stop compiling and the whole suite becomes unrunnable. The test is not touched here, matching the source branch. BoqaBlendedExomiserAnalyzerTest refers to BlendedResult too but still compiles, because #47 left its body inside a block comment.

TargetDisease becomes a sealed interface with two variants: Phenotype carries a disease with its HPO terms, Gene adds the gene that made the disease a candidate. Both carry observedHpoIds, so a target disease now arrives with its own annotations rather than being looked up in HpoDiseases by the code that scores it. CandidateDisease follows: Single and Blended drop their separate observed sets and read them off the target disease, and getMelded unions the components' terms instead of querying HpoDiseases. Call sites in BlendedCounter and BoqaBlendedExomiserAnalyser are updated to the new types. The wrapper keeps its current structure here; it is rewritten once the new pipeline exists.

BlendedCounter no longer holds diseases. It keeps only the ontology and gets both layers as term sets from its caller, through the new Counter.computeBoqaCountsFromDisease. BoqaCountsNew carries the four counts without a disease id or label, since a counter that is handed two sets does not know whose they are. Both layers are now expanded with getObservedWithAncestors. The disease layer was previously stored unexpanded, which made a perfect match score 335 false positives on OMIM:620851; the counter now agrees with BoqaSetCounter exactly on that pair. The old computeBoqaCounts is left returning a constant and getDiseaseIds returning an empty set, and the interface default of computeBoqaCountsFromDisease throws, so neither implementation of Counter satisfies the whole interface. The HP:0000118 filter is not carried over: isPhenotypicFeature is added but never called.

CandidateDiseaseNew forms the candidate list from TargetDisease.Gene alone: every anchor as a Single, every gene-disjoint pair as a Blended. Since commit "Split TargetDisease into Phenotype and Gene" put the HPO terms on the target disease, no HpoDiseases lookup is needed and none is done. A Blended holds its components as a set and no longer carries a melded disease. The concatenated OMIM:a-OMIM:b identifier that CandidateDisease built is dropped, along with the melded label, gene id and gene symbol. Nothing calls this class yet; CandidateDisease still serves the wrapper.

CandidateResult gains score() and counts() and becomes Comparable, ordering results by score descending with NaN last, so callers can sort without unwrapping the variants. DiseaseComponent moves from BoqaCounts to BoqaCountsNew. Blended changes from (components, finalDiseaseModel) to (components, counts, score). A blended result no longer names the melded disease, matching CandidateDiseaseNew, where a blend is its component set. The check that a Blended has at least two components is commented out rather than removed, so an empty blend is now constructible. BoqaBlendedExomiserAnalyser converts its counts to BoqaCountsNew to keep compiling; both the conversion and the surrounding loop go away when the wrapper moves to the new pipeline.

BoqaPatientAnalyzerNew takes a patient, a counter and a candidate list and returns ranked CandidateResults: raw log scores, softmax normalization, conversion to CandidateResult, the improvement filter for blends, sorting and the results limit. BoqaResultNew pairs BoqaCountsNew with a score and the candidate it belongs to, so the analyzer can reattach identity to counts that no longer carry it. This moves the analysis out of the Exomiser wrapper, which so far did the scoring itself, into the core module. Normalization runs over singles and blends together, so scores are spread across n + n(n-1)/2 candidates and do not sum to 1 once blends are filtered. computeBoqaResults sorts the list returned by computeBoqaResultsRawLog, which is unmodifiable, so the method throws UnsupportedOperationException for any non-empty candidate list. Util gains a rescaling variant for BoqaResultNew that nothing calls.

computeBlendedBoqaResults now builds a BlendedCounter and a CandidateDiseaseNew list and hands both to BoqaPatientAnalyzerNew. The scoring loop it used to run, including the per-candidate counts, the component rescoring and the improvement filter, is gone; all of it lives in the core module now. CandidateDisease and its melded TargetDisease are left behind unreferenced. The constructor still ingests HpoDiseases into a DiseaseData field that nothing reads, and the results limit is hard-coded to 100000.

Adds plantuml-generator-maven-plugin, scanning org.p2gx.boqa.core and writing boqa-core.puml with fields and methods hidden, to get a picture of the type graph the blended refactor has produced. The execution is bound to the compile phase, so it runs on every build rather than on request, and it writes into the project root, where the generated file is neither tracked nor ignored. The extra plugin also has to be downloaded before an offline build works.

TargetDisease.Phenotype and Gene become PhenotypeOnly and PhenotypeAndGene, naming the contrast rather than implying that the second variant is a gene. CandidateDiseaseNew and CandidateResult rename their variants likewise, and CandidateResult moves from the diseases package to analysis, next to the analyzer that builds it. The disease-data tests take a PhenotypeOnly prefix. CandidateResult also drops counts(): BlendedResult no longer carries the counts of the blend, which toCandidateResult now discards, while SingleResult still reaches its own through its DiseaseComponent. CandidateDisease is renamed along with the rest although nothing has referenced it since the wrapper moved to the new pipeline. That's the last commit. Once you've made it, I'll walk the eleven as a reviewer would and draft the PR description — the branch is currently 11 commits ahead of ph/pr47-retold and exactly matches PR50's head.

hansenp added 11 commits September 2, 2026 11:36

Navigation Menu

Uh oh!

Ph/pr50 retold - #52

Ph/pr50 retold#52
hansenp wants to merge 11 commits into
ph/pr47-retoldfrom
ph/pr50-retold

hansenp commented Sep 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

hansenp commented Sep 2, 2026

Sealed BOQA-blended refactor, in reviewable commits

What the change does

What this changes relative to #47

Findings

How this branch was built

Questions

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant