consolidate results based on is_set query parameter

[andrewsu]

The Reasoner API defines an is_set parameter (link) that should be used to help define our results merging behavior. One example that uses this is_set parameter is in NCATSTranslator/testing#134 -- this query is visually depicted below:

Currently in the results (https://arax.ncats.io/?r=604132c0-b073-49ad-be84-902231915061), BTE has three separate records where pazopanib is in n3 -- each record has a different gene in n0 (HDAC2, TP53, and ALK).

However, the intent of the is_set parameter in the query is to indicate that those three results be combined into a single results object. Although it's shown for a different SmallMolecule (Rosmarinic Acid), the ARAX results at the same link above shows the desired behavior:

related to our other issues on combining results: #331 and #164

[ariutta]

Does this change #331, such that we should only combine results if the is_set parameter is specified?

[andrewsu]

hmmm, are you suggesting that the specific case of #331 could be solved by a generic solution to this issue if we require that is_set is true for multi-ID nodes? Would that greatly simplify the implementation? If so, we could discuss this with the rest of the consortium...

[ariutta]

Yes

[ariutta]

In that case, should I make a pull request to just solve #331, or should I wait for you to confirm with the rest of the consortium? The way you phrased it in your comment is very well stated. If the consortium says yes, we could solve both this issue and #331 with a simpler solution than the one I came up with for #331 alone.

[andrewsu]

I'm crafting the question to the consortium now. So let's give them a day or two to see if there is clear consensus there...

[andrewsu]

Just got confirmation that we should indeed handle #331 as a special case of the is_set usage described in this issue. So, I will close #331, and please implement the generic solution based on the assumption that is_set will be True if multiple IDs in the query are to be collapsed into a single result. Good catch, @ariutta!

[ariutta]

@marcodarko, the query graph @andrewsu provided in issue #331 appears to surface a possible issue with QEdge.isReversed. In my tests, e01 has isReversed() == true when the query graph node n02 has the property ids. Without the property ids, isReversed() == false. Is this expected behavior?

What's confusing is that e00 always has isReversed() == false, regardless of whether the query graph node n00 has the property ids.

Update: this may well be expected behavior. If true, I'll update some code in query_results.js to handle this case.

{
  "message": {
    "query_graph": {
      "edges": {
        "e00": {
          "subject": "n00",
          "object": "n01"
        },
        "e01": {
          "subject": "n01",
          "object": "n02"
        }
      },
      "nodes": {
        "n00": {
          "ids": [
            "DRUGBANK:DB00215",
            "DRUGBANK:DB01175",
            "DRUGBANK:DB00472",
            "DRUGBANK:DB00176",
            "DRUGBANK:DB00715",
            "DRUGBANK:DB01104"
          ],
          "categories": ["biolink:SmallMolecule"]
        },
        "n01": {
          "categories": ["biolink:Gene"]
        },
        "n02": {
          "ids": ["DOID:5844", "DOID:1936", "DOID:3393"],
          "categories": ["biolink:Disease"]
        }
      }
    }
  }
}

[colleenXu]

@ariutta that sounds right to me? The edge manager queries edges based on which node has less IDs. So the first edge to execute will be reversed e01: 3 IDs -> Gene and it'll likely return > 6 Gene IDs. That means the second edge will be e00 not reversed: 6 IDs -> Gene

But if n02 doesn't have any IDs on it, then BTE only has 1 way to execute it....e00 not reversed for 6 IDs -> Gene and then e01 not reversed for Gene IDs -> Disease

[ariutta]

@colleenXu, there still appears to be a discrepancy of some sort. I created a test to demonstrate:
https://github.com/biothings/bte_trapi_query_graph_handler/blob/ba086e4d7dc1553cba5007f629f34473112157de/__test__/integration/helper.test.js#L196

In both of these tests, I would expect n1 to be associated with NCBIGene:3778, n2 with MONDO:0011122 & n3 with a second instance of NCBIGene:3778. That's what we get for the first test, but not for the second test.

In the second test, it appears the directionality is reversed for helper._getInputID, but not for helper._getInputQueryNodeID. If the directionality were to be consistently reversed for edge2, we'd still expect n2 to be associated with MONDO:0011122. But in this case, n2 is associated with NCBIGene:3778.

Is there anything wrong how I formatted these records?

Edit: yes, it appeared I had to change record2 by switching $input and $output. Compare this record:
https://github.com/biothings/bte_trapi_query_graph_handler/blob/300657d788be3e8de045aaba63d63a315947f694/__test__/integration/QueryResult.test.js#L353-L381
vs. this record:
https://github.com/biothings/bte_trapi_query_graph_handler/blob/300657d788be3e8de045aaba63d63a315947f694/__test__/integration/QueryResult.test.js#L544-L572

If ids is specified for the "last" query node, the query_edge.js code will automatically switch $input and $output for the relevant records. For the test, we need to do this manually.

[ariutta]

This issue may be addressed in my branch results_assembly_tree_traversal_is_set. Note that the code changes in this branch are primarily intended for improved performance (10x in some cases!) in results assembly, taking advantage of the tree constraint (issue 359). The handling of this is_set issue is a smaller part tacked on.

This code block is where consolidation happens. Note that consolidation is needed for two different cases. This subsection of that block is specific to the case of the is_set param. If we aren't ready to use all the changes from this branch, we could potentially port just the is_set consolidation code to the current production codebase, but it wouldn't be 100% straightforward.

There are several things to verify before we can merge this branch.

Specific to this issue about is_set params:

• Is the way I specified is_set in the test records correct? (@tokebe may be the best positioned to answer the questions about the implementation of is_set in the code?)
• Is this method correct for determining whether an is_set param has been specified? If yes, it should probably get moved to another file like helper.js.
• related to the point above, my code assumes only one QNode of a QEdge can have an is_set param. And because the current codebase automatically reverses an edge if the subject QNode doesn't have a curie but the object QNode does (relevant code), my code further assumes if a QNode has an is_set param, that QNode is an input (related code). update: now only making these assumptions: a QEdge can have 0, 1 or 2 QNodes having is_set params. If the QEdge has just 1 QNode with an is_set param, that QNode is the input (or will be reversed by the QEdge code to be treated as the input). See this code section.
• My tests were only for cases where QNodes with is_set params were either the root node or a leaf node. (update: removed this assumption)

Unrelated to this issue but still needs to be verified (more related to issue 359):

• This code assumes either the records passed in have been de-duplicated or it doesn't matter if we return duplicate identical results. @marcodarko, does that sound OK? (update: now handles duplicates)
• This code also assumes the query graph doesn't have cycles. Should we add a validation step to the query graph parser to be run before the query is executed? Note that it's probably possible to change the _getPreresults method to handle cycles, but it would add complexity.
• I created a new test case that fails, and I'm not sure why. Is it because I'm creating the records incorrectly, or is it a problem with the algorithm? Does it matter?

[tokebe]

@ariutta Looking at the is_set params questions:

• Given your assumptions, I'm pretty sure you have the node parameters set/checked correctly. input.obj[0] is definitely the right place, and I don't think there's a case where there will ever be an obj[1] to worry about?
• Based on the description for is_set, it does look like it's possible for both nodes to have is_set, which does change the behavior (if both have is_set=true, then then every subject must connect to at least one object, if I'm reading it right). I'm not sure that it's necessarily safe to assume only one node will have is_set=true, but that goes a bit outside my current sphere of knowledge...