Combine qedge

[rjawesome]

Should complete all objectives of this issue

[rjawesome]

TODO: fix code to pass tests

[tokebe]

@rjawesome I know you're in-progress with this one and have several other issues on your plate (that I've recently commented on as well), however I just want to clarify that of the BTE issues assigned to you, this one should be the highest priority (excepting anything that @andrewsu might assign you as higher priority).

[rjawesome]

@tokebe this should be mostly good to go.

one thing with the inputHasResolved() function, the qEdge uses

 hasInputResolved() {
    if (this.isReversed()) {
      return this.object.hasEquivalentIDs();
    }
    return this.subject.hasEquivalentIDs();
  }

while qXEdge uses

  hasInputResolved() {
    return !(Object.keys(this.input_equivalent_identifiers).length === 0);
  }

I am currently using the 2nd implementation in the new QEdge class but that is causing some problems in one of the tests

FAIL __test__/integration/QEdge2BTEEdgeHandler.test.js
  ● Testing NodeUpdateHandler Module › Testing _getCuries function › test edge with input node annotated should return an empty array

    TypeError: entities.map is not a function

      200 |         let categories = [];
      201 |         Object.values(this.equivalentIDs).map((entities) => {
    > 202 |         entities.map((entity) => {
          |                  ^
      203 |             categories = [...categories, ...entity.semanticTypes];
      204 |         });
      205 |         });

      at map (src/query_node.js:202:18)
          at Array.map (<anonymous>)
      at QNode.getCategories (src/query_node.js:201:43)
      at map (src/update_nodes.js:20:53)
          at Array.map (<anonymous>)
      at NodesUpdateHandler._getCuries (src/update_nodes.js:15:13)
      at Object.<anonymous> (__test__/integration/QEdge2BTEEdgeHandler.test.js:41:37)

equivalentIDs is

{ 'NCBIGene:1017': { db_ids: { NCBIGene: [Array], SYMBOL: [Array] } } }

Which implementation should i use?

[tokebe]

This behavior should be merged -- qXEdge has input_equivalent_identifiers and output_equivalent_identifiers, while qEdge has input and output equivalentIDs, which should become the same thing. This will involve some fiddling with multiple other files in the package to all align with one naming scheme.

Previously, "input" and "output" seem to have been used to avoid confusion between "subject" and object" when reversing a qEdge. Given this, I think it makes sense for the new representation to be accessed along the lines of qEdge.getSubject().getEquivalentIDs().

For now, we can keep the use of input with respect to hasInputResolved and such.

[tokebe]

That said, I'd like to ask @colleenXu's opinion regarding the possibility of changing getSubject() and getObject() to getInput() and getOutput() with respect to sub-querying (and therefor representation in qEdge/qNode and possibly in Records), or if we should try to standardize around using just subject and object.

[colleenXu]

I'm not fully following this conversation...but I think this has to do with the conversation here #93 (comment) on whether subject/object or input/output is less-confusing.

Reviewing the convo, it seems like both are confusing. I was concerned with keeping track of the directionality of QEdge vs "direction we sub-query in", I think...and whatever words we use to make that clear is good with me...

[tokebe]

Ok, I propose we use input/output to exclusively refer to "direction we're sub-querying in", while in all other cases we use subject/object. I think it also makes sense to not touch Records as those can be thought of as just a knowledge triple independent of sub-query direction.

@rjawesome TLDR: A few new implementation things:

• Remove getSubject() and getObject(), instead using the existing getInputNode() and getOutputNode() (which reference the internal .subject and .object properties which shouldn't change)
• Replace uses of qEdge.input_equivalent_identifiers with qEdge.getInputNode().getEquivalentIDs() So that equivalent identifiers are stored in and retrieved from nodes (this will involve searching the package for references and changing some other files)
• After this, see what changes around the failing test. It's possible there are two slightly different structures between the current input_equivalent_identifiers and node.equivalentIDs that will have to be sorted out -- you can try setting breakpoints to pause during the test when using the main branch as a point of comparison to find if the structure is different.

[codecov]

Codecov Report

Merging #126 (cf80a4e) into main (ed09bbe) will decrease coverage by 1.15%.
The diff coverage is 66.36%.

❗ Current head cf80a4e differs from pull request most recent head 606e8b1. Consider uploading reports for the commit 606e8b1 to get more accurate results

@@            Coverage Diff             @@
##             main     #126      +/-   ##
==========================================
- Coverage   70.79%   69.64%   -1.16%     
==========================================
  Files          26       25       -1     
  Lines        2332     2306      -26     
==========================================
- Hits         1651     1606      -45     
- Misses        681      700      +19     

Impacted Files	Coverage Δ
src/query_edge.js	52.12% <51.94%> (-38.79%)	⬇️
src/query_graph.js	66.66% <69.62%> (-2.77%)	⬇️
src/edge_manager.js	65.73% <70.58%> (ø)
src/index.js	77.86% <72.22%> (+0.09%)	⬆️
src/qedge2apiedge.js	58.69% <72.72%> (ø)
src/cache_handler.js	82.73% <81.81%> (ø)
src/query_node.js	82.82% <81.91%> (-0.68%)	⬇️
src/batch_edge_query.js	83.90% <88.23%> (+0.18%)	⬆️
src/update_nodes.js	90.90% <100.00%> (-0.52%)	⬇️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[rjawesome]

@tokebe Should be fixed along with this PR

[tokebe]

Given that the id used in the record constructor is now usually expected to be in the info object, it would make sense to restructure the constructor to accept a frozen qEdge directly.

Looking great otherwise 👍

[rjawesome]

Done!

[tokebe]

Closing as feature update has been rolled into #129