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

qualifiers in meta knowledge graph #610

Merged
merged 12 commits into from
May 22, 2023
Merged

qualifiers in meta knowledge graph #610

merged 12 commits into from
May 22, 2023

Conversation

rjawesome
Copy link
Contributor

@rjawesome rjawesome commented Apr 5, 2023

#595
Qualifiers are just read from the existing qualifiers in yaml.

Currently doesn't merge edges with same input/output/predicate but different qualfiers in the meta knowledge graph.

Copy link
Member

@tokebe tokebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far so good, but a few notes:

  • qualifier_type_id should have the biolink: prefix stripped and re-added to ensure it's present and non-duplicated
  • applicable_values should have biolink: prefix stripped, and only added if the qualifier_type_id includes "predicate"

Regarding your question about supportedLookups():

supportedLookups() should add the complete qualifier set from the given template (provided the template has qualifiers) to each supported lookup it generates. In controllers/meta_knowledge_graph.js, the step that calls supportedLookups() should use this new qualifier information alongside the subject/predicate/object to attempt to match to an existing edge. If it matches, add the knowledge type inferred to that edge, otherwise make a new edge with just knowledge type inferred and the information from the supported lookup.

@rjawesome
Copy link
Contributor Author

@tokebe Can you give a sample template file. I don't see it in the bte trapi repo but maybe I'm missing it?

@tokebe
Copy link
Member

tokebe commented Apr 6, 2023

Follow the imports: controllers/meta_knowledge_graphs.js imports supportedLookups() from query_graph_handler, which imports it from query_graph_handler/src/inferred_mode/template_lookup.js, which shows that templates are read from templateGroups.json.

You'll need to edit the function in template_lookup.js as well to actually grab the qualifier sets.

@rjawesome
Copy link
Contributor Author

rjawesome commented Apr 6, 2023

Oh I was looking for the json file in the wrong repo...

@rjawesome
Copy link
Contributor Author

Should be fixed, also a new PR on query graph handler

@colleenXu
Copy link
Collaborator

colleenXu commented Apr 11, 2023

@tokebe

I have a question specific to the v1/meta_knowledge_graph response. When a MetaEdge can be done as lookup or creative-mode....did we want to have one operation with "knowledge_types": ["inferred", "lookup"] or two?

With the current PR code, two separate MetaEdges are made. I saw these two:

        {
            "subject": "biolink:SmallMolecule",
            "predicate": "biolink:treats",
            "object": "biolink:Disease",
            "knowledge_types": [
                "lookup"
            ]
        },


        {
            "subject": "biolink:SmallMolecule",
            "predicate": "biolink:treats",
            "object": "biolink:Disease",
            "knowledge_types": [
                "inferred"
            ]
        },

@colleenXu
Copy link
Collaborator

colleenXu commented Apr 11, 2023

@tokebe @rjawesome

Regarding association functionality...

  • Could we put it into a separate PR / separate set of PRs? I'm thinking that we'll test this functionality later, when we put the info into the SmartAPI yamls
  • could the SmartAPI yaml (x-bte operation) property name be changed from "association_id" -> "biolink_association_type"?
  • Note: In the SmartAPI yaml, the property's value will probably be just the term like EntityToDiseaseAssociation. But in the endpoint response, we will want this to have the biolink-prefix ("association": "biolink:EntityToDiseaseAssociation")

@colleenXu colleenXu changed the title predicate/association in meta knowledge graph association/qualifiers in meta knowledge graph Apr 11, 2023
@colleenXu
Copy link
Collaborator

And I changed the title because I assumed we meant "qualifiers".

I'm done with my quick testing of the "qualifiers functionality" for #595. I posed my two questions / chunks of feedback above, and I guess we'll wait to hear about the "qualifier-set merging" from the TRAPI team / Translator (and we'll track that discussion in the issue).

It looks great overall!

@rjawesome rjawesome changed the title association/qualifiers in meta knowledge graph qualifiers in meta knowledge graph Apr 12, 2023
@rjawesome
Copy link
Contributor Author

Association has been moved into its own PR. Will address Colleen's comments in that PR

@tokebe
Copy link
Member

tokebe commented Apr 13, 2023

@tokebe

I have a question specific to the v1/meta_knowledge_graph response. When a MetaEdge can be done as lookup or creative-mode....did we want to have one operation with "knowledge_types": ["inferred", "lookup"] or two?

With the current PR code, two separate MetaEdges are made. I saw these two:

        {
            "subject": "biolink:SmallMolecule",
            "predicate": "biolink:treats",
            "object": "biolink:Disease",
            "knowledge_types": [
                "lookup"
            ]
        },


        {
            "subject": "biolink:SmallMolecule",
            "predicate": "biolink:treats",
            "object": "biolink:Disease",
            "knowledge_types": [
                "inferred"
            ]
        },

We want edges that are exactly the same other than knowledge_types to be merged, so it's one edge with "knowledge_types": ["inferred", "lookup"].

@rjawesome
Copy link
Contributor Author

Should be fixed

@tokebe
Copy link
Member

tokebe commented Apr 14, 2023

Fix currently has inferred duplicating on some edges, like below:

{
      "subject": "biolink:SmallMolecule",
      "predicate": "biolink:affects",
      "object": "biolink:Gene",
      "qualifiers": [
        {
          "qualifier_type_id": "biolink:causal_mechanism_qualifier",
          "applicable_values": [
            "inverse_agonism"
          ]
        },
        {
          "qualifier_type_id": "biolink:object_aspect_qualifier",
          "applicable_values": [
            "activity"
          ]
        },
        {
          "qualifier_type_id": "biolink:object_direction_qualifier",
          "applicable_values": [
            "decreased"
          ]
        },
        {
          "qualifier_type_id": "biolink:qualified_predicate",
          "applicable_values": [
            "biolink:causes"
          ]
        }
      ],
      "knowledge_types": [
        "lookup",
        "inferred",
        "inferred"
      ]
    },

@tokebe
Copy link
Member

tokebe commented Apr 14, 2023

(Also, I merged the latest from main, you'll have to update your workspace by cloning this repo fresh and using npm run setup)

@rjawesome
Copy link
Contributor Author

Shoudl be fixed

@colleenXu
Copy link
Collaborator

Hmmm....What is shown above is actually "incorrect" because we wouldn't actually run creative-mode on a QEdge if all those qualifiers were specified as constraints on that QEdge.

I think some logic may be needed to figure out when a lookup TRAPI MetaEdge should have the inferred string added to knowledge_types (aka "merging"). Maybe the logic is similar to what's done to match a QGraph QEdge to creative-mode templates...

Some thoughts I have:

  • if a lookup edge has more qualifiers specified (larger set of type_ids), then it is representing MORE SPECIFIC stuff than the creative-mode template and creative-mode won't run. So we shouldn't add the inferred string...
  • If the lookup edge had the exact same set of qualifier type_ids/values as a creative-mode template (and the subject/predicate/object matches too), then we want to add the inferred string
    • I doubt that we have a lookup edge for SmallMolecule-affects-Gene with the exact qualifier set increased activity_or_abundance. But if we do, then we'd want to add the inferred string.
    • If we don't have that lookup edge, we'll make an inferred-only TRAPI MetaEdge...
  • but we also currently have some ambiguity in our creative-mode behavior: creative-mode will happen if the inferred QEdge's qualifier set is more general. Do we want to add the inferred string in these situations?
    • For example, I know we have a lookup edge where SmallMolecule-affects-Gene with NO qualifiers. We could decide to add the inferred string there, since creative-mode will run if an inferred QEdge like that was provided...
    • Another example: I doubt that we have a lookup edge SmallMolecule-affects-Genewith the exact qualifier setincreased. But if we did, we could decide to add the inferred` string there since creative mode would run...
    • I'm not sure we ever got clarification from the rest of Translator on whether this ambiguity in behavior was okay. We discussed it as "point 2" earlier this year here: my question, Jackson reply, Andrew reply

@tokebe
Copy link
Member

tokebe commented Apr 17, 2023

@colleenXu I'm not convinced this logic is necessary... based on the way the poll is going, it's looking like Translator want the more general "all possible qualifiers" option, in which case such ambiguity is expected. It wouldn't be read as "we support creative mode on this edge with these qualifiers", so much as "we support creative mode on this edge with some subset of these qualifiers" and then we have no duty to be more clear.

@tokebe
Copy link
Member

tokebe commented Apr 27, 2023

Looks like this is almost ready to go -- I'm noticing a couple cases of duplicate "inferred" in some of the metaEdges, however:

{
      "subject": "biolink:ComplexMolecularMixture",
      "predicate": "biolink:affects",
      "object": "biolink:Protein",
      "qualifiers": [
        {
          "qualifier_type_id": "biolink:object_aspect_qualifier",
          "applicable_values": ["activity_or_abundance"]
        },
        {
          "qualifier_type_id": "biolink:object_direction_qualifier",
          "applicable_values": ["increased", "decreased"]
        }
      ],
      "knowledge_types": ["inferred", "inferred"]
    }

@tokebe tokebe merged commit 0a68f6e into main May 22, 2023
@tokebe tokebe deleted the knowledge-graph-update branch October 25, 2023 20:05
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.

3 participants