RTXteam / RTX

Software repo for Team Expander Agent (Oregon State U., Institute for Systems Biology, and Penn State U.)
https://arax.ncats.io/
MIT License
33 stars 21 forks source link

physically_interacts_with edges showing up as bidirectional arcs in RTX UI #683

Closed saramsey closed 5 months ago

saramsey commented 4 years ago

In response to this picture:

Screen Shot 2020-03-13 at 8 38 33 AM

@edeutsch wrote:

I’m slightly dismayed that physically_interacts_with edges go both directions again. I thought to stomped that out several times in the past, but it seems to be back. Not a real problem really, but seems undesirable.

I would like to review the history of how this issue was dealt with in previous iterations of the RTX system, before I can make a recommendation for how to avoid this problem in the current (ARAX) system.

saramsey commented 4 years ago

Again, I don't know the history of this. But my feeling is that this graph visualization accurately reflects the data model in Neo4j. As such, it doesn't seem to be a bug. I mean, the KG in this case has bidirectional arcs.

saramsey commented 4 years ago

Is there a way to represent an undirected edge in a Swagger KnowledgeGraph object?

edeutsch commented 4 years ago

Two comments:

1) In response to "would like to review the history of how this issue was dealt with in previous iterations", I don't know where this is, but I will point out that when you perform the same query via the QueryGraphReasoner after entering a QueryGraph in the GUI, there's only one edge: https://arax.rtx.ai/beta/?m=1517

2) Is there a way to represent an undirected edge in a Swagger KnowledgeGraph object: This is one for @isbluis . I don't know what is possible with the graphing plugin that is is using (Cytoscape). Right now, there is always a source_id and a target_id. There could be a flag for "undirected" or "bidirectional". I don't know if the grapher can do that? In the official 0.9.2, there is not a way to do it.

I vaguely recall some discussion about just choosing a convention.. by convention small molecules interact with the usually larger proteins. I dunno..

amykglen commented 4 years ago

re:

but I will point out that when you perform the same query via the QueryGraphReasoner after entering a QueryGraph in the GUI, there's only one edge: https://arax.rtx.ai/beta/?m=1517

I think the query graph you used there is slightly different from the one in demo Example 1 (that generates the bidirectional edges) - you specified a type for the edge (physically_interacts_with), while the demo example does not. I think QueryGraphReasoner functions bidirectionally if no edge type is given, and otherwise is directional. (I don't really get why it is designed that way, but believe that is so.)

edeutsch commented 4 years ago

aha, you're right! If you specify the type in the add_qedge, then there's only one edge int the result: https://arax.rtx.ai/beta/?m=1518

So I suppose the easiest fix is to switch to that in the demo notebooks.

amykglen commented 4 years ago

yeah, think so. for now at least! that behavior is certainly something we can redefine when we (post-demo) move away from using QueryGraphReasoner in expand(). (seems like ultimately we wouldn't want directionality to be dependent on whether or not an edge type is specified - see #627 for discussion on edge directionality in expand.)

saramsey commented 4 years ago

Is there any action item for me (resultify or KG2 related), coming out of this issue?

edeutsch commented 4 years ago

Maybe we should have an edge cleaner functionality, perhaps as part of expand().

It might do a couple things, but for starters remove all redundant cases of edges of the same type from the same knowledge source. If they're pointing in the same direction for some reason, just remove all but one. If they're pointing in opposite directions, remove one. I suggest the node type earlier in the alphabet is the staying source node (e.g. source chemical_substance physically interacts with target protein stays; opposite direction is removed). If the edges come from different knowledge sources then I suppose they should stay. For now at least. This functionality should be controlled by an option. I vote enabled by default.

saramsey commented 4 years ago

Maybe we should have an edge cleaner functionality, perhaps as part of expand().

It might do a couple things, but for starters remove all redundant cases of edges of the same type from the same knowledge source. If they're pointing in the same direction for some reason, just remove all but one. If they're pointing in opposite directions, remove one. I suggest the node type earlier in the alphabet is the staying source node (e.g. source chemical_substance physically interacts with target protein stays; opposite direction is removed). If the edges come from different knowledge sources then I suppose they should stay. For now at least. This functionality should be controlled by an option. I vote enabled by default.

Two edges of the same type from the same knowledge source (pointing in the same direction) for the same pair of subject/object nodes shouldn't happen, at least for KG2. If such a case happens, I would regard it as a bug in KG2 and I would propose to fix it there.

As for bidirectional edges with the same predicate, my concern is that what if some downstream tool/module is doing reasoning on the result_graph? Wouldn't that make automated reasoning more difficult, since now the reasoner will have to know that certain predicates are automatically bidirectional, and which ones are not. For example, suppose that node A and node B are bidirectionally connected by positively_regulates; this is a common situation with feedback loops in signaling networks. But it is by no means automatic, i.e., in KG2, most pairs of nodes connected by a positively_regulates arc will not have a bidirectional connection between them. On the other hand, certain predicates like physically_interacts_with are sort of automatically bidirectional based on the predicate semantics. So if the ARAX reasoning code removes one of the edges of a bidirectional pair of edges between a pair of nodes, how is a downstream reasoning tool/module to know whether or not that edge has bidirectional semantics or not? I claim that this will make reasoning on (or automated processing of) the result_graph more difficult. I think it would be better to do pruning of bidirectional edges in the presentation layer (i.e., in the UI layer).

saramsey commented 4 years ago

Maybe we should have an edge cleaner functionality, perhaps as part of expand(). It might do a couple things, but for starters remove all redundant cases of edges of the same type from the same knowledge source. If they're pointing in the same direction for some reason, just remove all but one. If they're pointing in opposite directions, remove one. I suggest the node type earlier in the alphabet is the staying source node (e.g. source chemical_substance physically interacts with target protein stays; opposite direction is removed). If the edges come from different knowledge sources then I suppose they should stay. For now at least. This functionality should be controlled by an option. I vote enabled by default.

Two edges of the same type from the same knowledge source (pointing in the same direction) for the same pair of subject/object nodes shouldn't happen, at least for KG2. If such a case happens, I would regard it as a bug in KG2 and I would propose to fix it there.

As for bidirectional edges with the same predicate, my concern is that what if some downstream tool/module is doing reasoning on the result_graph? Wouldn't that make automated reasoning more difficult, since now the reasoner will have to know that certain predicates are automatically bidirectional, and which ones are not. For example, suppose that node A and node B are bidirectionally connected by positively_regulates; this is a common situation with feedback loops in signaling networks. But it is by no means automatic, i.e., in KG2, most pairs of nodes connected by a positively_regulates arc will not have a bidirectional connection between them. On the other hand, certain predicates like physically_interacts_with are sort of automatically bidirectional based on the predicate semantics. So if the ARAX reasoning code removes one of the edges of a bidirectional pair of edges between a pair of nodes, how is a downstream reasoning tool/module to know whether or not that edge has bidirectional semantics or not? I claim that this will make reasoning on (or automated processing of) the result_graph more difficult. I think it would be better to do pruning of bidirectional edges in the presentation layer (i.e., in the UI layer).

Why can't the UI just display a bidrectional edge as an undirected edge (no arrow) between the nodes?

saramsey commented 4 years ago

Having said all that, I guess I could live with an optional edge-cleaner functionality. Where would it be implemented? I suppose in ARAX-filter would make the most sense? Or were you thinking in ARAX-expand? In that case, what if ARAX-overlay produces (downstream of expand) an edge with bidirectional semantics? I mean, every node-pair test that overlay does is pretty much symmetric right, so by default the arcs from ARAX-overlay have bidirectional semantics, right?

isbluis commented 4 years ago

I guess I could do the edge collapsing on my end at render-time, but would need very clear guidelines on how to go about it (e.g. same edge type, and all attributes, etc...?). Would not want to lose any potentially important data/values when merging edges. And quite easy to display edges with no arrows.

edeutsch commented 4 years ago

If you're happy to try this on the presentation layer, this would be fine by me. I am persuaded by Steve that this may be the best place to put it.

We can start with very clear guidelines:

It may get more complicated, but this is the lion's share of the problem now. If there are others, I don't notice them yet, because they are swamped by this one.

isbluis commented 4 years ago

Happy in the sense that it is indeed possible and not hard to do. But always concerned to be putting any business logic on the presentation layer -- a classic no-no; ideally done upstream.

saramsey commented 5 months ago

I think this is no longer an issue, right? OK to close, @isbluis and @edeutsch ?

edeutsch commented 5 months ago

yeah, I think we can close this, thanks.