Should we continue to allow query_graph_reasoner to be called via the DSL?

[saramsey]

Note from SAR: The issue described below was due to a bug in my code (fixed code can be found in test08 in ARAX_resultify.py in the demo branch). But it brought up a larger question of whether we still need to allow QueryGraphReasoner to be accessed via the DSL. Have renamed this Issue accordingly, but I am preserving the original "bug report" below for posterity.

For some reason, when the answer method in the class QueryGraphReasoner is called (based on what I am seeing in the demo branch), the result of the query does not get put in themessage.knowledge_graph object as I might expect. This is causing problems because the ARAX_resultify method __resultify expects to find the knowledge graph in the message object. So the following test code is failing:

Tentatively categorizing this as a bug but I am not 100% sure.

[dkoslicki]

@saramsey This works without error in python ARAX_query.py 17 with the query:

query = {"previous_message_processing_plan": {"processing_actions": [
            "create_message",
            "add_qnode(name=DOID:731, id=n00, type=disease, is_set=false)",
            "add_qnode(type=phenotypic_feature, is_set=false, id=n01)",
            "add_qedge(source_id=n00, target_id=n01, id=e00)",
            "expand(edge_id=e00)",
            'resultify(ignore_edge_direction=true)',
            "return(message=true, store=false)"]}}

However, it appears to only return one result (instead of a list of results, given that both qnodes have is_set=false).

A few things to note here:

1. Use expand not query_graph_reasoner
2. qnode name is either a CURIE or a Neo4j node.name
3. qnode id= should be a string you choose, not a DOID (by convention, but I don't think imposed anywhere, just might mess with the parser)

[dkoslicki]

I don’t think query_graph_reasoner() should be called in any processing_actions so would actually vote to close this as a non-issue (I.e. use expand instead). But then again, I don’t know if there was plans to allowquery_graph_reasoner()` as a processing action. @amykglen might want to weigh in.

[edeutsch]

Early examples that I put together did use the QueryGraphReasoner prior to the readiness of Expand. But now that Expand is ready, I think I would agree that we should scrub out calls to QueryGraphReasoner in our examples and testing. Should we completely remove its ability to be called? I don't know. It might be handy to be able to call it, but there are potential pitfalls, I guess.

[saramsey]

Thanks, guys. Restating this issue as a question: "Should we allow QueryGraphReasoner to be called via the DSL?". My fixed test code is here, which works nicely:

If we decide to deprecate query_graph_reasoner from the DSL, I will eliminate _test08 from the ARAX_resultify.py module.

[edeutsch]

An interesting test of resultify() is: do you get the same exact result with and without resultify() in this sequence? Because the query_graph_reasoner() already computes is own results. So in this case resultify() is superfluous. But it is interesting ask if the same answer is achieved.

[saramsey]

Where did we leave off with this issue? Close it out?

[edeutsch]

Ancient history, I think.