A more complete Qualifiers implementation

[edeutsch]

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]

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)?

[edeutsch]

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]

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):

1. In the Edgeobject:

   • 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)

2. In the QualifierConstraintobject:

   • 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 Edgeand QualifierConstraintobjects - 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]

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]

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]

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]

@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!

[edeutsch]

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

Removed constrained_predicate https://github.com/NCATSTranslator/ReasonerAPI/pull/330/commits/ef1a94f17bb8c392c61f19e24f4a80d9324cdc18

Rename Edge.qualifier_set to Edge.qualifiers https://github.com/NCATSTranslator/ReasonerAPI/pull/330/commits/4d527286d78c3cc4ba41484b1a354a029a12508a

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