Blended BOQA Exomiser integration by pnrobinson · Pull Request #47 · P2GX/BOQA

pnrobinson · 2026-08-03T06:23:49Z

No description provided.

pnrobinson · 2026-08-03T06:27:37Z

@leokim-l This PR is intended to work with exomiser/Exomiser#648
@julesjacobsen
Checkout this branch, do "mvn install" (to put the latest library version into .m2) and then the Exomiser version should compile. We will still need to integrate the command so we can run it from the command line (@julesjacobsen ?). We should now create some VCF files for actually testing this (separate repo).

leokim-l

All in all, I think this PR adds new interesting parts, addresses a lot of issues of our code structure, adds useful classes and tries to redesign several parts. However, it seems to add lots of parts that duplicate already existing solutions and it has some minor bugs here and there.

I believe if we decide the time has come to redesign a large portion of the code it would be best to sit down and talk about requirements, domain objects and their relationships and map this out together. Alternatively, I could try to take what is in here and adapt it within the branch I was working on. This would roughly mean using the sealed interfaces and the new classes to obtain one implementation of Counter, one way to write out results (and not multiple overlapping classes), one way to write disease IDs and Labels without resorting to String concatenation.

leokim-l · 2026-08-03T11:37:43Z

+    public static AlgorithmParameters defaultParams() {
+        return AlgorithmParameters.create(DEFAULT_ALPHA, DEFAULT_BETA);
+    }
+


create() with no arguments does exactly this, though a dedicated method might be better. If we want this dedicated defaultParams() method, then we should get rid of

BOQA/boqa-core/src/main/java/org/p2gx/boqa/core/algorithm/AlgorithmParameters.java

Lines 42 to 43 in 8cb3869

double a = (alpha != null) ? alpha : DEFAULT_ALPHA;

double b = (beta != null) ? beta : DEFAULT_BETA;

leokim-l · 2026-08-03T11:41:29Z

+            .map(TermId::of)
+            .collect(toSet());
+        return new DefaultPatientData(randomId, observedTidSet);
+    }


Why is this method in the interface? To the best of my understanding it should be moved to the class DefaultPatientData. Also, we should consider adding something like

BOQA/boqa-core/src/main/java/org/p2gx/boqa/core/patient/PhenopacketData.java

Lines 54 to 55 in 8cb3869

.map(traverser::getPrimaryTermId)

.filter(Objects::nonNull) // If old HPO is used without a term, avoids the program crashing

so we avoid misalignments between the HPO Ontology being used by BOQA and the terms being passed from the outside world. We trust Exomiser, of course, but BOQA should internally in its classes enforce this somehow, in my opinion.

leokim-l · 2026-08-03T11:46:55Z

There are now two TargetDisease classes, this one and the one in the diseases directory. There is no clear "better one", both have a little bit more with respect to their minimum common denominator...

leokim-l · 2026-08-03T12:33:52Z

+        TargetDisease t1 = diseasePair.get(0);
+        TargetDisease t2 = diseasePair.get(1);
+        String diseaseId = t1.diseaseId() + "-" + t2.diseaseId();
+        String diseaseLabel = t1.diseaseLabel() + "-" + t2.diseaseLabel();
+        String geneId = t1.geneId() + "-" + t2.geneId();
+        String symbol = t1.geneSymbol() + "-" + t2.geneSymbol();


I think we should abandon this strategy for generating IDs and labels, it has already backfired... In one place we write classes and methods that do string concatenation, then someplace else we need to undo it (for sure this happened in the analysis code). Something similar also happened in phenopacket2prompt, where a regex based naming convention based on an ID managed to generate LOTS of issues in downstream analysis. I would prefer being cautious when unpacking ID fields and generating new IDs manually editing strings. We could generate a sort of structured ID, maybe something like DiseaseInfo, containing a List or Set of (ID, Label) pairs.

leokim-l · 2026-08-03T14:36:37Z

+    }
+
+    /**
+     * COPIED FROM BoqaSetCounter. After testing we should make this a default in the interface!


Why should this be a default in the interface? I would not want to put source code that may change in an interface, I believe

leokim-l · 2026-08-03T15:15:47Z

Test differences seem harmless and simply done to conform to the new signatures, all good here

leokim-l · 2026-08-03T15:19:17Z

Copied from here (just for easier tracing)

leokim-l · 2026-08-03T15:23:32Z

leokim-l · 2026-08-03T15:39:05Z

+                TermId diseaseId = TermId.of(s.diseaseId());
+                  HpoDisease hpoDisease = hpoDiseaseMap.get(diseaseId);
+                  if (hpoDisease != null) {
+                    Set<TermId> observed = new HashSet<>();
+                    for (HpoDiseaseAnnotation hda : hpoDisease.presentAnnotations() ){
+                        observed.add(hda.id());
+                    }
+                    diseaseLayers.put(diseaseId, observed);
+                  }
+               }
+               case CandidateDisease.Blended b -> {
+                 List<TargetDisease> list = b.components(); 
+                  Set<TermId> observed = new HashSet<>();
+                  for (TargetDisease td: list) {
+                    TermId diseaseId = TermId.of(td.diseaseId());
+                  HpoDisease hpoDisease = hpoDiseaseMap.get(diseaseId);
+                  if (hpoDisease != null) {
+                    for (HpoDiseaseAnnotation hda : hpoDisease.presentAnnotations() ){
+                        observed.add(hda.id());
+                    }


This seems wrong, disease layers as per BoqaSetCounter do not only contain observed HPOs, but also the induced graph above them. The counter is agnostic to a single disease or a bleneded one, there should not be differences beyond (maybe) in bookkeeping. I like the idea of using sealed interfaces and changing some parts of our code that are a bit messy, but this re-implementation is missing:

Filtering for terms below Phenotypic Abnormality

BOQA/boqa-core/src/main/java/org/p2gx/boqa/core/algorithm/BoqaSetCounter.java

Line 64 in c89782b

.filter(phenotypicAbnormalities::contains)

Adding the induced HPO terms to diseaseLayers

BOQA/boqa-core/src/main/java/org/p2gx/boqa/core/algorithm/BoqaSetCounter.java

Line 66 in c89782b

return ontologyTraverser.initLayer(diseasePhenotypes);

I'd suggest, if changing the Counter is necessary, to adapt the existing implementation to the sealed interface.

leokim-l · 2026-08-03T15:53:32Z

Looks OK, though consider the comments I had in the components that are used in here, and that I have not explicitly tested this. I would also hesitate to throw out what @hansenp wrote without having taken the time to understand it better. It seems a bit convoluted, but it might contain some things we are missing.

BOQA/boqa-core/src/main/java/org/p2gx/boqa/core/analysis/BoqaBlendedExomiserAnalyzer.java

Line 62 in c89782b

public List<BlendedResult> analyze(PatientData patient, Set<TargetDisease> anchorDiseases, int resultsLimit) {

Maybe rather than removing it we mark it @deprecated and wait for him to come back from vacation before removing it?

Blended BOQA Exomiser integration

d4f8d70

pnrobinson requested a review from leokim-l August 3, 2026 06:23

pnrobinson added 2 commits August 3, 2026 08:51

Updating maven action to be version-agnostic for running CLI

3f5b283

fixing CLI jar identification

8cb3869

leokim-l reviewed Aug 3, 2026

View reviewed changes

hansenp mentioned this pull request Aug 25, 2026

Ph/pr47 retold #51

Draft

hansenp mentioned this pull request Sep 2, 2026

Ph/pr50 retold #52

Draft

Navigation Menu

Uh oh!

Blended BOQA Exomiser integration - #47

Blended BOQA Exomiser integration#47
pnrobinson wants to merge 3 commits into
developfrom
bboqa

pnrobinson commented Aug 3, 2026

Uh oh!

pnrobinson commented Aug 3, 2026

Uh oh!

leokim-l left a comment •

edited

Loading

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026

Uh oh!

leokim-l Aug 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	double a = (alpha != null) ? alpha : DEFAULT_ALPHA;
	double b = (beta != null) ? beta : DEFAULT_BETA;

	.map(traverser::getPrimaryTermId)
	.filter(Objects::nonNull) // If old HPO is used without a term, avoids the program crashing

Uh oh!

Conversation

pnrobinson commented Aug 3, 2026

Uh oh!

pnrobinson commented Aug 3, 2026

Uh oh!

leokim-l left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leokim-l Aug 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leokim-l left a comment •

edited

Loading

leokim-l Aug 3, 2026 •

edited

Loading