Skip to content

Bugfix, filter nulls in getPrimaryTermId#38

Open
leokim-l wants to merge 2 commits intodevelopfrom
lc/bugfix-observed-hpo-not-in-ontology
Open

Bugfix, filter nulls in getPrimaryTermId#38
leokim-l wants to merge 2 commits intodevelopfrom
lc/bugfix-observed-hpo-not-in-ontology

Conversation

@leokim-l
Copy link
Collaborator

bugfix, keep allowing null but filter for it and expand warning description. Otherwise running the code with old HPO might crash, which is a bit too strong

@leokim-l leokim-l assigned leokim-l and hansenp and unassigned leokim-l Feb 12, 2026
@leokim-l leokim-l marked this pull request as draft March 9, 2026 11:44
@leokim-l
Copy link
Collaborator Author

leokim-l commented Mar 9, 2026

Daniel pointed out this method should not be static (tied to class, not instance), or if called without an instance (which triggers the constructor) it will lead to a null pointer exception. Since it is closely related, I will fix this in this PR and then mark it ready for review again.

public static TermId getPrimaryTermId(TermId t){
TermId primaryTermId = hpo.getPrimaryTermId(t);

…s and changed method call in constructor to reference an instance, adapted tests and made them uniform by using a per-class lifcycle
@leokim-l leokim-l marked this pull request as ready for review March 9, 2026 16:10
@ielis
Copy link
Contributor

ielis commented Mar 10, 2026

The code looks IMO better, I like that the Ontology is not static anymore.

However, if would be superior to only have one constructor in PhenopacketData - a constructor that simply takes all the fields. The logic that depends on hpo can then be moved either into a static method on PhenopacketData or to some other class (the class for mapping a phenopacket to PatientData).

This would simplify making naive PhenopacketData instances for testing, and IMO structure the code better.

But, it's a minute change. Please implement if you find it useful.

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.

3 participants