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

association property addition is buried in MetaQualifier PR #426

Open
colleenXu opened this issue Apr 11, 2023 · 4 comments
Open

association property addition is buried in MetaQualifier PR #426

colleenXu opened this issue Apr 11, 2023 · 4 comments
Milestone

Comments

@colleenXu
Copy link
Contributor

colleenXu commented Apr 11, 2023

I noticed that there's something that seems unrelated to MetaQualifiers in the MetaQualifier PR: this new association property on MetaEdges (see below).

In https://github.com/NCATSTranslator/ReasonerAPI/pull/387/files:

        association:
          type: object
          $ref: '#/components/schemas/BiolinkEntity'
          description: >-
            The Biolink association type (entity) that this edge represents.
            Associations are classes in Biolink
            that represent a relationship between two entities.
            For example, the association 'gene interacts with gene'
            is represented by the Biolink class,
            'biolink:GeneToGeneAssociation'.  If association
            is filled out, then the testing harness can
            help validate that the qualifiers are being used
            correctly.
          example: biolink:ChemicalToGeneAssociation

I am wondering if:

  • this should be a separate entry in the changelog to direct peoples' attention to it
  • association will inherit the BiolinkEntity schema and will be type:string, not type:object. Is type:object then an error in the schema?
  • there are any requirements related to this change...It isn't a required property, but is there a push by the SRI team @sierra-moxon @RichardBruskiewich to have this on all MetaEdges? And is this for KPs only?
  • there are any possible "merging" issues like there are for qualifiers.
    • Would a KP have two sets of edge data that match two different Association types....but fall under the same S-P-O combo? Is that even possible?
    • Would two KPs assign different Association types to the same S-P-O combo? Is that possible? Would this leave an ARA tool that uses both KPs unclear on what Association to assign for that S-P-O combo?
@sierra-moxon
Copy link
Member

sierra-moxon commented Apr 13, 2023

Good to add this discussion to the TRAPI call agenda :)

  • "push by the SRI team @sierra-moxon @RichardBruskiewich to have this on all MetaEdges": it will help us validate the edges that do employ qualifiers more easily if we have the association class that the provider of the edge is trying to provide. But it is optional because we know this is a big lift. It would be nice to see some folks returning it to see if we can better validate their edges. I'm not sure I understand why an ARA that could provide this property, wouldn't? But I would definitely like to see the challenges here more clearly so we can make the specification more robust. :)
  • "there are any possible 'merging' issues like there are for qualifiers." : the definition for what makes an edge unique should be clarified, I agree. To me, SPO+association alone, does not make an edge unique. But the full definition of what does make an edge unique needs discussion in the context of merging. SPO+Qs+negation+publications would be a start in my opinion (though everyone has different edge attributes that might also need consideration). I don't think association would contribute to the definition of unique if the baseline for uniqueness was already SPOQs+negation+publications.

@RichardBruskiewich
Copy link
Contributor

Thanks @colleenXu for your comments. I'm leaving @sierra-moxon to mainly comment on this (as she has above). I do note, however, your mention of the type: object versus $ref. In fact, we should likely remove the type: object property and move the $ref to after the example: tag, since the $ref effectively effectively overwrites any subschema tags which follow it (see see $ref and Sibling Elements on https://swagger.io/docs/specification/using-ref/).

I've now submitted a PR for this: #428. @sierra-moxon, if we need to adjust anything else in this particular MetaEdge.association property, we can perhaps use this PR to achieve this?

@vdancik vdancik added this to the v1.5 milestone May 25, 2023
@edeutsch
Copy link
Collaborator

Still not sure that this was addressed in #428.
@colleenXu is this still a concern?
Not a schema issue, but an implementation discussion still needs to happen.

@colleenXu
Copy link
Contributor Author

#428 only addressed 1 of my original post's bullet points (on inheritance).

I'm not sure that the other points have been addressed:

  • raising awareness that this is there ("hidden" in another feature)
  • any requirements / deadlines from SRI team
  • exploring if there are any merge issues (similar to the meta-qualifiers discussion)

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

No branches or pull requests

5 participants