Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only add is_set if ids are expanded #134

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Only add is_set if ids are expanded #134

merged 1 commit into from
Feb 6, 2023

Conversation

tokebe
Copy link
Member

@tokebe tokebe commented Feb 1, 2023

Addresses biothings/biothings_explorer#555.

Test cases:

Issue query

Using http://localhost:3000/v1/smartapi/8f08d1446e0bb9c2b323713ce83e2bd3/query

{
  "message": {
    "query_graph": {
      "nodes": {
        "n0": {
          "ids": ["UniProtKB:P35968", "UniProtKB:Q07912"],
          "categories": ["biolink:Gene"]
        },
        "n1": {
          "categories": ["biolink:SmallMolecule"]
        }
      },
      "edges": {
        "e1": {
          "subject": "n0",
          "object": "n1"
        }
      }
    }
  }
}

query that should expand
{
  "message": {
    "query_graph": {
      "nodes": {
        "n0": {
          "categories": ["biolink:ChemicalEntity"]
        },
        "n1": {
          "ids": ["MONDO:0018997"],
          "categories": ["biolink:DiseaseOrPhenotypicFeature"],
          "name": "noonan syndrome"
        }
      },
      "edges": {
        "eA": {
          "subject": "n0",
          "object": "n1",
          "predicates": ["biolink:treats"]
        }
      }
    }
  }
}

CC @colleenXu for quick testing before deployment to dev.

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #134 (9970d13) into main (e81dee9) will decrease coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
- Coverage   60.75%   60.62%   -0.14%     
==========================================
  Files          25       25              
  Lines        2431     2433       +2     
==========================================
- Hits         1477     1475       -2     
- Misses        954      958       +4     
Impacted Files Coverage Δ
src/index.js 70.61% <100.00%> (-1.32%) ⬇️

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

@tokebe tokebe merged commit d37f519 into main Feb 6, 2023
@colleenXu
Copy link
Contributor

Pasted from my Slack message on 2/6 (internal lab Slack link):

Belated feedback after testing the is_set/node-expansion PR:

  • It looks okay.
  • I'm not sure that I tested all possible cases:
    • I tested cases starting with 1 ID on a QNode, and with multiple IDs on a QNode
    • I checked that is_set: true is correctly added / results show correct merging when a QNode's IDs are expanded to more IDs.
      • works when QNode starts out without is_set set
      • works when QNode starts out with is_set:false -> it gets set to true
      • works when QNode starts out with is_set:true
    • I checked that is_set is NOT set automatically to true when a QNode starts out with multiple IDs (and no expansion happens).
Example query
  • n0 shouldn't get expanded/shouldn't be is_set:true
  • n1 should be expanded and have is_set:true added
{
    "message": {
        "query_graph": {
            "edges": {
                "e01": {
                    "subject": "n0",
                    "object": "n1"
                }
            },
            "nodes": {
                "n0": {
                    "ids": ["NCBIGene:8131", "UniProtKB:Q12980", "NCBIGene:9681", "UniProtKB:O75140", "NCBIGene:10641", "UniProtKB:Q8WTW4"],
                    "categories": ["biolink:Gene", "biolink:Protein"]
                },
                "n1": {
                    "ids": ["MONDO:0014716", "MONDO:0020310", "MONDO:0012611", "MONDO:0019341"],
                    "categories": ["biolink:Disease"]
                }
            }
        }
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants