NCATSTranslator / ReasonerAPI

NCATS Biomedical Translator Reasoners Standard API
35 stars 28 forks source link

Should we use the fancy supporting graph infrastructure for subclass inference? #414

Open edeutsch opened 1 year ago

edeutsch commented 1 year ago

There was some musing about whether simple subclass inference as performed by KPs should also use the auxiliary / supporting graph infrastructure.

This seemed appealing to some. But others felt that there was no need to take something that is already working and change it so substantially. We are undecided. This issue is a placeholder to consider this further and make a decision.

This issue illustrates the ongoing conversation: https://github.com/NCATSTranslator/Feedback/issues/117

colleenXu commented 1 year ago

Our team is still thinking this through, and we have asked for help in the reasonerapi Slack channel. I'm pasting my post from that channel below.

This post has links to graphics/slides for the result.node_bindings.query_id implementation (Q1) vs using auxiliary-graphs / needing to create edges (Q2).


Topic 1: how to represent a result when ID-expansion is done?

AKA an entity-curie on a QNode was expanded to include ontology-descendants. In the migration guide, we saw that query_graph cannot be modified, and result.node_bindings must only include QNodes from query_graph.

Q1: Is it still okay to use result.node_bindings.query_id to represent that ID-expansion was done? See my example here

Q2: If the answer to Q1 is no, then what is the recommended way to represent the info in my example?

Q3: If the answer to Q2 is to use auxiliary graphs + create edges ... then is "nesting" okay?

colleenXu commented 1 year ago

And now for my impressions (not representing my teams as a whole):

A. implementing result.node_bindings.query_id should be much easier for our team (we haven't done it yet because we have been waiting for more guidance)

B. while our team can most likely implement an auxiliary-graph / creating-edges method, I'm not enthusiastic about it...

C. I understand that users find it confusing when descendant-terms are used, when that's not what they asked for. I wonder...

edeutsch commented 1 year ago

There is a PR here: https://github.com/NCATSTranslator/ReasonerAPI/pull/420/commits/e9c41d59a8e9e30f28f9491b4bbb970b80bae8d6

that proposes to implement this. I believe @vdancik and @andrewsu and @dkoslicki were interested in commenting on this proposal? See also @colleenXu comments above.

colleenXu commented 1 year ago

@edeutsch I think the example in the PR needs some edits:

edeutsch commented 1 year ago

thanks! @uhbrar would you see if you can fix?

uhbrar commented 1 year ago

Yes, I've gone ahead and fixed that. Thanks!