A more complete Qualifiers implementation #330

edeutsch · 2022-05-21T06:45:07Z

No description provided.

mbrush · 2022-06-01T00:11:24Z

TranslatorReasonerAPI.yaml

          type: array
          description: >-
-            A list of contraints applied to a query edge.
+            A list of attribute contraints applied to a query edge.
            If there are multiple items, they must all be true (equivalent
            to AND)
          items:
            $ref: '#/components/schemas/QueryConstraint'


Now that we have the ability to constrain queries based on values of Attributes and Qualifiers, consider renaming the QueryConstraint object 'AttributeConstraint' or 'AttributeQueryConstraint'? And then name the object that defines a constraint based on a Qualifier value a 'QualifierConstraint' or 'QualifierQueryConstraint'. (This object is currently named 'QualifierSet' in this PR). This would make naming more clear and consistent.

I think we can use AttributeConstraint and QualifierConstraint to distinguish the two kinds of constrains.

mbrush · 2022-06-01T00:13:49Z

TranslatorReasonerAPI.yaml

+            predicate it is qualifying via the
+            QualiferSet.qualified_predicate property.
+          items:
+            $ref: '#/components/schemas/QualifierSet'


As noted above, consider renaming 'QualifierSet' --> 'QualifierConstraint' or 'QualifierQueryConstraint', to be more clear and consistent with the name of the object that holds a constraint definition based on Attribute values.

I was initially skeptical, but now agree that 'QualifierSet' --> 'QualifierConstraint' may perhaps be a little better.

After pondering further, I'm convinced that QualifierConstraint is better. I have updated the PR.

mbrush · 2022-06-01T00:15:49Z

TranslatorReasonerAPI.yaml

+          description: >-
+            The value associated with the type of the qualifier, drawn from
+            a set of controlled values by the type as specified in
+            the Biolink model (e.g. 'decreased' or 'increased' for the type


Consider making the example here consistent with the example of a qualifier_type_id above (where you used 'subject_aspect'). So here, say "(e.g. 'expression' or 'abundance' for the qualifier type 'subject_aspect', etc)." And then change the example below from 'decreased' to 'expression'.

good idea, fixed.

mbrush · 2022-06-01T00:16:18Z

TranslatorReasonerAPI.yaml

+          example: decreased
+          nullable: false
+      required:
+        - type


Should the required 'type' property here read 'qualifier_type_id'?

yes, definitely, fixed, thanks.

mbrush · 2022-06-01T00:29:15Z

TranslatorReasonerAPI.yaml

+    QualifierSet:
+      additionalProperties: false
+      description: >-
+        A set of Qualifiers that act together to add nuance to an assertion.


The definition here seems too general - is this because the notion of a QualifierSet is menat to be used elsewhere, for purposes other than constraining a query?
If not, consider the proposed renaming/reframing of this object, and updating definition to something like:
"Defines a query constraint based on the type and value of a Qualifier attached to an edge. e.g. can constrain a "ChemicalX - affects - ?Gene" query to return only edges where ChemicalX specifically affects the 'expression' of the Gene, by constraining on the qualifier_type "object_aspect" with a value of "expression".

Qualifier is used both for QEdges and Edges. So it used both for querying and for conveying assertions. That is why the definition is slanted more toward the assertion side. In the context of queries, it specifies the constraints on the desired assertions. So okay as is?

True that 'Qualifier' object is used in asserting knowledge and querying for it. But the definition here is for a 'QualifierSet' object, which we agreed above to rename 'QualifierConstraint' (because its express purpose is to allow constraining queries based on the values of Qualifiers on an edge).
So given that we are now defining a 'QualifierConstraint' object, it makes sense to me to update the definition so that it clearly conveys this purpose of this object in constraining queries.

I understand now. Yes, agreed. Updated with minor edits. See if the committed version seems agreeable.

mbrush · 2022-06-01T00:37:54Z

TranslatorReasonerAPI.yaml

+          items:
+            $ref: '#/components/schemas/Qualifier'
+          nullable: false
+        qualified_predicate:


'qualified_predicate' is the name of an edge property defined in Biolink. It may be confusing to have a TRAPI property with the same name. Consider the name 'constrained_predicate'?

I am neutral, but fine to change if it seems less conflicting with Biolink.

I understand better now and after pondering further, I'm convinced that constrained_predicate is better. I have updated the PR.

mbrush · 2022-06-01T00:44:32Z

TranslatorReasonerAPI.yaml

+      description: >-
+        A set of Qualifiers that act together to add nuance to an assertion.
+      properties:
+        qualifiers:


If we rename QualifierSet --> QualifierConstraint, I would consider renaming this field from 'qualifiers' --> 'qualifier_set'. 'qualifiers' is fine, but I feel like 'qualifier_set' indicates more clearly that the qualifiers in this array are to be considered together in constraining the query.

I was initially skeptical, but now agree that 'qualifier_set' may perhaps be a little better.

After pondering further, I'm convinced that qualifier_set is better. I have updated the PR.

mbrush · 2022-06-02T16:09:38Z

TranslatorReasonerAPI.yaml

+          description: >-
+            The predicate that is to be qualified in cases where there
+            are multiple predicates in a query and there thus might be
+            ambiguity.


In this initial draft of an object to support Qualifier-based query constraints, I would also include a boolean 'not' property, as exists for the QueryConstraint object used for Attribute-based constraints . Key for the basic functionality of excluding specific qualifiers from a result.

hmm, I think this needs more discussion. I view "not" and "exclude" as two very different things. In my view, "not" should be used as:

Which drugs do not treat disease X? (e.g. clinical trials were run and the the result was that the drug does not treat X)

while "exclude" has a different functionality:

Which drugs are related to disease X, excluding those that are contraindicated for disease X

This exclude flag was proposed to be a direct QEdge property. although no consensus was reached and it remains ARAX-only functionality

The "not" property was also deferred until such a time as we would carefully think through and document how this would be implemented (regarding inheritance and inference, etc.)

So I think we should avoid this unless we have more discussion.

This suggestion was motivated by an example query Valdo provided that asks "What compounds increase the activity or phosphorylation of PPARA but not its abundance". But as you point out, negation/exclusion are tricky and perhaps it is best to leave this out for now, until we look into it more closely.

okay, let's leave it out for now

- update value to qualifier_value - Correct the names of the required fields - update the qualifier_value example and description to be consistent with the qualifier_type_id

edeutsch · 2022-06-21T20:06:12Z

So this is where I see the current PR:

and this is what @mbrush is suggesting:

I am fine with these changes except for the "not". That might need some more discussion.

mbrush · 2022-06-21T20:30:56Z

I drafted some example qualifier-based Associations in the document here - and included a few queries illustrating the qualifier constraint proposal above (with modifications I suggested). These queries are at the end of the document - and I think align well with the bottom part of Eric's diagram.
@edeutsch maybe take a look at my examples document to confirm (you can ignore the 'not' element in the final query example)?

… with Biolink qualifier term

edeutsch · 2022-06-22T00:35:14Z

okay, everyone, after updating the PR based on @mbrush excellent comments, we now have this structure in the PR:

Please everyone examine carefully and review. This is our release candidate for qualifier support in TRAPI.

mbrush · 2022-06-22T18:06:01Z

The proposal looks pretty good to me. I might consider minor tweaks to two property names and definitions (but not a big deal if we keep things as they are):

In the Edge object:
- change the name of the property qualifier_set back to qualifiers (to be consistent with the name of the attributes property here)
- change definition of this property to "a set of Qualifiers that act together to add nuance or detail to the statement expressed in an Edge" (the term 'statement' rather than 'assertion' is more consistent with the language we use in our documentation of the qualifier-based approach)
In the QualifierConstraint object:
- Keep the name qualifier_set , but update the definition so that it describes the role qualifiers play in a Constraint (the present definition describes the role Qualifiers play in an Edge). The new def may be something like "a set of Qualifiers that serves to add nuance to a query, by constraining allowed values held by Qualifiers on queried Edges."

I understand that the current proposal is motivated to use the same property name (qualifier_set) and definition ("a set of qualifiers that together provide additional detailed nuance to the assertion beyond what the subject-predicate-object can provide") in both the Edge and QualifierConstraint objects - because these properties both hold a set of Qualifier objects. However these qualifiers serve different roles in these contexts, and to me this warrants different definitions that describe these distinct roles. That said, if folks still prefer to keep the same name and defs for these properties, I wont object.

edeutsch · 2022-06-22T18:40:21Z

Thanks for the review. I think your definition adjustments all seem sensible and I will make those unless there are objections. But I would like to discuss the property name a bit further:

change the name of the Edge property qualifier_set back to qualifiers (to be consistent with the name of the attributes property here)

It is true that you requested in the previous round to change QualifierConstraint.qualifiers to QualifierConstraint.qualifier_set, and then I went ahead and also changed Edge.qualifiers to Edge.qualifier_set even though you didn't suggest it. I did that for what I perceived as consistency. So I am surprised that you would have us change it back for consistency.

It seems to make more sense to me to keep both QualifierConstraint.qualifier_set and Edge.qualifier_set consistent since they are essentially the same thing. rather than aim for consistency between Edge.attributes and Edge.qualifier[_set|s].

I thought calling it qualifier_set rather made a lot of sense because the Qualifiers within a set really do work together closely. Whereas the attributes are mostly a pile of independent things (some are provenance, some are inherent properties, some are assigned attributes, etc.)

So I rather like qualifier_set. But I'm happy to be outvoted.

How about all readers that made it this far put in their votes?

Rocket for Edge.qualifier_set
Heart for Edge.qualifiers

mbrush · 2022-06-22T19:30:42Z

I am fine with either name for the Edge property. I don't feel like we need to use the same property name in the Edge and QualifierConstraint objects, since the referenced/nested Qualifier objects are serving different purposes in these different objects/contexts. I have a slight preference for Edge.qualifiers (to be consistent with the name of the Edge.attributes property in the same object). But am fine with Edge.qualifier_set too . . . esp if we update the QualifierConstraint.qualifier_set definition as proposed so it is clear what role Qualifiers are playing here. @edeutsch were you ok with the proposed changes to this definition?

edeutsch · 2022-06-23T04:34:50Z

edeutsch were you ok with the proposed changes to this definition?

@mbrush Yes, I think they're great. I was leaving a little time for others to chime in, but hearing none, the latest commit to the PR now uses your definitions. Seem good?

mbrush · 2022-06-29T01:13:40Z

@edeutsch looks good to me! Still slight preference for the name Edge.qualifiers over Edge.qualifier_set . . . but if Hearts vs Rockets vote above remains 1-1 we can keep it as is. Thanks for your work on this!

Removed constrained_predicate for now. It may come back in TRAPI 1.4 with more thought: ``` constrained_predicate: $ref: '#/components/schemas/BiolinkPredicate' description: >- The predicate that is to be qualified in cases where there are multiple predicates in a query and there thus might be ambiguity. nullable: true ```

edeutsch · 2022-06-30T18:26:04Z

okay, based on today's TRAPI call and follow-up with @mbrush, I have made the following changes to the PR:

Removed constrained_predicate
ef1a94f

Rename Edge.qualifier_set to Edge.qualifiers
4d52728

Did I do this correctly?
any further concerns?
If correct, we are aiming to merge later today. Please respond.

vdancik · 2022-06-30T21:30:11Z

TranslatorReasonerAPI.yaml

+            $ref: '#/components/schemas/Qualifier'
+          nullable: false
+      required:
+        - qualifiers


@edeutsch , shouldn't required field in QualifierConstraint contain qualifier_set instead of qualifiers ?

yes, absolutely, good catch! Fixed in PR with f4bcfc5
thanks!

edeutsch added 2 commits May 20, 2022 23:42

A more complete Qualifiers implementation

05ca040

explictly disallow additionalProperties in new classes

eceb723

mbrush reviewed Jun 1, 2022

View reviewed changes

mbrush reviewed Jun 2, 2022

View reviewed changes

This was referenced Jun 15, 2022

Support for qualifiers #325

Closed

Alternative support for qualifiers #326

Closed

Add QEdge.qualifier_constraints and rename to attribute_constraints #327

Closed

edeutsch added 2 commits June 15, 2022 11:23

Rename QueryConstraint -> AttributeConstraint

a111b7c

fixes to Qualifier

a098299

- update value to qualifier_value - Correct the names of the required fields - update the qualifier_value example and description to be consistent with the qualifier_type_id

edeutsch added 4 commits June 21, 2022 17:10

Change QualifierSet to QualifierConstraint

70e0eec

Change qualifiers to qualifier_set

6dd6099

Change qualified_predicate to constrained_predicate to avoid conflict…

a7bbf7a

… with Biolink qualifier term

Updated QualifierConstraint description as per Matt

0b95383

edeutsch requested review from mbrush and vdancik June 22, 2022 00:31

Updated qualifier_set definitions as per Matt B

9a4e9f4

vgardner-renci requested a review from vemonet June 23, 2022 12:24

vgardner-renci requested review from tziomics, webyrd, andrewsu, brettasmi, CaseyTa, edgargaticaCU, kennethmorton, marcdubybroad, MarkDWilliams, RichardBruskiewich and veenhouse June 23, 2022 12:24

brettasmi approved these changes Jun 28, 2022

View reviewed changes

mbrush approved these changes Jun 29, 2022

View reviewed changes

kennethmorton approved these changes Jun 29, 2022

View reviewed changes

CaseyTa approved these changes Jun 30, 2022

View reviewed changes

edgargaticaCU approved these changes Jun 30, 2022

View reviewed changes

edeutsch added 2 commits June 30, 2022 11:19

Rename Edge.qualifier_set to Edge.qualifier as discussed at today's call

4d52728

vdancik reviewed Jun 30, 2022

View reviewed changes

fix required element to the correct name

f4bcfc5

vdancik approved these changes Jul 1, 2022

View reviewed changes

edeutsch merged commit b470a3b into 1.3 Jul 1, 2022

edeutsch deleted the edeutsch-qualifiers-3 branch July 6, 2022 03:56

A more complete Qualifiers implementation #330

A more complete Qualifiers implementation #330

Conversation

edeutsch commented May 21, 2022

mbrush Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrush Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edeutsch commented Jun 21, 2022

mbrush commented Jun 21, 2022

edeutsch commented Jun 22, 2022

mbrush commented Jun 22, 2022

edeutsch commented Jun 22, 2022

mbrush commented Jun 22, 2022 • edited Loading

edeutsch commented Jun 23, 2022

mbrush commented Jun 29, 2022 • edited Loading

edeutsch commented Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

edeutsch Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

mbrush Jun 1, 2022 •

edited

Loading

mbrush Jun 1, 2022 •

edited

Loading

mbrush Jun 1, 2022 •

edited

Loading

mbrush Jun 1, 2022 •

edited

Loading

mbrush Jun 21, 2022 •

edited

Loading

mbrush Jun 1, 2022 •

edited

Loading

mbrush Jun 1, 2022 •

edited

Loading

mbrush Jun 21, 2022 •

edited

Loading

mbrush commented Jun 22, 2022 •

edited

Loading

mbrush commented Jun 29, 2022 •

edited

Loading

edeutsch commented Jun 30, 2022 •

edited

Loading

edeutsch Jul 1, 2022 •

edited

Loading