Skip to content

Conversation

dylan-sa
Copy link
Contributor

@dylan-sa dylan-sa commented Mar 30, 2023

Fixes #506.

This PR includes changes related to the removal of inverse properties from gist:

  • All inverses removed. (Inverse working group has proposed which directions to keep in gist.)
  • Relevant axioms have been tweaked to include only the properties that will remain in gist. A common change was to switch out a property restriction with its inverse property. For example:
a owl:Restriction ;
owl:onProperty gist:isBasisFor ;
owl:someValuesFrom gist:Event ;

becomes

a owl:Restriction ;
owl:onProperty [ owl:inverseOf gist:isBasedOn ] ;
owl:someValuesFrom gist:Event ;
  • If the direction we are removing contained more information than its inverse, the additional information was maintained in some cases where useful. (See, e.g., isGovernedBy.)

Breakdown (updated after 5/1 working group meeting):

Properties retained in gist Inverse properties removed from gist
hasDirectPart isDirectPartOf
hasDirectSubTask isDirectSubTaskOf
hasDirectSuperCategory hasDirectSubCategory
hasMember isMemberOf
hasNavigationalParent hasNavigationalChild
hasPart isPartOf
hasSubTask isSubTaskOf
hasSuperCategory hasSubCategory
isAbout isDescribedIn
isAffectedBy affects
isBasedOn isBasisFor
isGeographicallyContainedIn containsGeographically
isGovernedBy governs
isIdentifiedBy identifies
isRecognizedBy recognizes
occupiesGeographically isGeographicallyOccupiedBy
occupiesGeographicallyPermanently isGeographicallyPermanentlyOccupiedBy
precedes follows
precedesDirectly followsDirectly

Shared spreadsheet with final summary and proposal history. (SA folks can access.)

@dylan-sa dylan-sa self-assigned this Mar 30, 2023
@dylan-sa dylan-sa added impact: major Non-backward compatible (changes inferences; e.g., adding a restriction, domain, range) topic: inverses labels Mar 30, 2023
@dylan-sa dylan-sa marked this pull request as ready for review March 30, 2023 19:12
@rjyounes
Copy link
Collaborator

rjyounes commented Apr 3, 2023

@marksem @uscholdm @dylan-sa On further thought, describes is the proper inverse of isDescribedBy; isAbout means something a bit different. Consider: the anatomy of my foot is described by Gray's Anatomy. We can say that the book describes the bone structure of my foot, but not that it is about my foot. One solution is to replace isDescribedBy with describes (for brevity) and keep isAbout as well.

@uscholdm
Copy link
Contributor

uscholdm commented Apr 4, 2023

@marksem @uscholdm @dylan-sa On further thought, describes is the proper inverse of isDescribedBy; isAbout means something a bit different. Consider: the anatomy of my foot is described by Gray's Anatomy. We can say that the book describes the bone structure of my foot, but not that it is about my foot. One solution is to replace isDescribedBy with describes (for brevity) and keep isAbout as well.

This is a vaid distinction, but let's treat that as a separate issue "Do we add a gist:descibes property". If there was a demonstrated need for both, maybe. But I'd need some convincing. Is about seems fine for most purposes.

@uscholdm
Copy link
Contributor

uscholdm commented Apr 4, 2023

Breakdown:

It would be super nice for this table to be alpha sorted so one can look things up.

@rjyounes
Copy link
Collaborator

rjyounes commented Apr 4, 2023

@uscholdm

This is a valid distinction, but let's treat that as a separate issue "Do we add a gist:descibes property".

Normally I like to keep issues atomic, as you suggest, but this one is different. If they are not true inverses, and we remove one of them, we are removing a concept that is no longer expressible in gist. The short-term solution is to keep both and remove the inverse axiom, and consider later whether we can drop one of them.

@uscholdm
Copy link
Contributor

uscholdm commented Apr 4, 2023

we are removing a concept that is no longer expressible in gist

I do not that is what is happening. We are removing a property whose only meaning ever was to be the inverse of isAbout. It was poorly named.

@rjyounes
Copy link
Collaborator

rjyounes commented Apr 4, 2023

I do not that is what is happening. We are removing a property whose only meaning ever was to be the inverse of isAbout. It was poorly named.

OK, valid point. I withdraw my proposal and have added new issue #814.

Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

It looks like the serializer has not run on these commits. @dylan-sa You should check your setup. Did you run tools/setup.cmd after you pulled the latest from develop?

@dylan-sa
Copy link
Contributor Author

dylan-sa commented Apr 4, 2023

It looks like the serializer has not run on these commits. @dylan-sa You should check your setup. Did you run tools/setup.cmd after you pulled the latest from develop?

You're right--this wasn't set up correctly. Fixed with latest commit.

@dylan-sa
Copy link
Contributor Author

dylan-sa commented Apr 4, 2023

It would be super nice for this table to be alpha sorted so one can look things up.

Good idea--I have tweaked the ordering. 👍

@dylan-sa
Copy link
Contributor Author

dylan-sa commented Apr 4, 2023

describes is the proper inverse of isDescribedBy

Agree that this is a better name for the inverse--worth considering in a separate issue.

@Jamie-SA
Copy link
Contributor

Jamie-SA commented Apr 4, 2023

I am curious about the decision to keep gist:governs and remove gist:isGovernedBy. I looked at the spreadsheet posted by Dylan on 3/28/2023 and I'm unclear on what the column names really mean (the Group Consensus column contradicts the Recommend Keeping column).

But my main comment is that the implemented choice seems to go against our general rule of thumb of lower cardinality. I would think gist:isGovernedBy is goes from the leafs and points to the root of a tree (hierarchy). But you've chosen the other direction. This seems to be the higher cardinality relation. And it isn't obvious to me that there is a specific semantic or use case benefit to choosing gist:governs instead.

@dylan-sa
Copy link
Contributor Author

dylan-sa commented Apr 4, 2023

I looked at the spreadsheet posted by Dylan on 3/28/2023 and I'm unclear on what the column names really mean (the Group Consensus column contradicts the Recommend Keeping column).

Sorry for confusion on the spreadsheet--Group Consensus indicates what the working group settled on keeping, whereas Recommend Keeping is from an earlier version of the spreadsheet. The table at the top of the PR is a better source for the working group proposal.

@rjyounes
Copy link
Collaborator

rjyounes commented Apr 4, 2023

@Jamie-SA We brought other considerations to bear on the decision. In this case, due to the verbosity of isGovernedBy, we opted for governs. It's more a matter of balancing criteria than having hard-and-fast rules. In this particular case I could personally go either way.

@dylan-sa dylan-sa marked this pull request as draft April 6, 2023 21:21
@dylan-sa dylan-sa requested review from Jamie-SA, hmoore-sa and rjyounes and removed request for uscholdm and marksem April 7, 2023 19:20
@dylan-sa dylan-sa marked this pull request as ready for review April 7, 2023 19:35
@dylan-sa
Copy link
Contributor Author

dylan-sa commented Apr 7, 2023

@rjyounes @Jamie-SA @hmoore-sa - After some more adjustments, this one is ready for review. Would appreciate everyone's thoughts when you get a chance to look.

Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

Good work - it looks like you caught everything.

@uscholdm
Copy link
Contributor

While looking at this I see that the choice was made to keep containsGeographically and remove isGeographicallyContainedIn. I would like to suggest a couple of reasons why the opposite choice might makes sense.

containsGeographically means the same thing as hasPart between georegions and may be short lived. For now, we don't want have isGeographicallyContainedIn which means isPartOf. So Im in favor of having it as it is for now.
THere was a special reason for occupiesGeographically going the other way, I do not recall.

@uscholdm
Copy link
Contributor

While looking at this I see that the choice was made to keep containsGeographically and remove isGeographicallyContainedIn. I would like to suggest a couple of reasons why the opposite choice might makes sense.

containsGeographically means the same thing as hasPart between georegions and may be short lived because it is not adding anything much. In any event, we don't want to have isGeographicallyContainedIn which means isPartOf. So Im in favor of having it as it is for now. THere was a special reason for occupiesGeographically going the other way, I do not recall.

Copy link
Contributor

@Jamie-SA Jamie-SA left a comment

Choose a reason for hiding this comment

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

Approved, but with comments.

### Major Updates

- Removed all inverse properties. Issue [#506](https://github.com/semanticarts/gist/issues/506).
- For each pair of inverses, the property deemed clearest, simplest, and/or most useful was retained.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this should list the removed properties? Yes, there is a link to the issue, but if someone just scans the release notes it might be nice for them to be here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this should list the removed properties? Yes, there is a link to the issue, but if someone just scans the release notes it might be nice for them to be here.

Yes, I strongly agree

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed.

Comment on lines -3171 to -3125
gist:hasDirectSubCategory
a owl:ObjectProperty ;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the removal of hasDirectSubCategory in the table in the issue or PR discussions. It's removal should be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good point. I'll add that to the release notes.

@rjyounes rjyounes self-requested a review April 27, 2023 22:14
Copy link
Collaborator

@rjyounes rjyounes left a comment

Choose a reason for hiding this comment

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

Just follow Jamie's suggestion of adding the list to the release note. Then it's good to go.

@dylan-sa
Copy link
Contributor Author

I can get behind Jamie's suggestion to go with isGeographicallyContainedIn.

Michael does make a fair point that it's similar in meaning to isPartOf, which we are dropping; but relating regions using the smaller-to-larger direction has proved useful for regions hierarchies.

@uscholdm
Copy link
Contributor

I can get behind Jamie's suggestion to go with isGeographicallyContainedIn.

Michael does make a fair point that it's similar in meaning to isPartOf, which we are dropping; but relating regions using the smaller-to-larger direction has proved useful for regions hierarchies.

I think we should get consensus before making yet another flip/flop.

M.

@rjyounes
Copy link
Collaborator

rjyounes commented Apr 28, 2023 via email

@rjyounes rjyounes marked this pull request as draft April 28, 2023 16:48
@dylan-sa
Copy link
Contributor Author

dylan-sa commented May 1, 2023

Decision after 5/1 working group meeting is to go w/ isGeographicallyContainedIn over containsGeographically, for reasons Jamie has described. I've pushed a commit to make the switch. If I could get one last look at the latest commit from @hmoore-sa , @Jamie-SA, or @rjyounes, I believe this one is ready to go.

@dylan-sa dylan-sa marked this pull request as ready for review May 1, 2023 19:49
@hmoore-sa
Copy link
Contributor

Hi Dylan, Took a look, mostly focusing on the isGeographicallyContainedIn changes, and looks like you got everything as far as I can see. Continuing to be signed off!

@rjyounes rjyounes force-pushed the issue-506-remove-inverses branch from 4ca767f to def36eb Compare May 3, 2023 19:33
@rjyounes rjyounes merged commit 5c5da62 into develop May 3, 2023
@rjyounes rjyounes deleted the issue-506-remove-inverses branch May 3, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact: major Non-backward compatible (changes inferences; e.g., adding a restriction, domain, range) topic: inverses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove inverses unless there's a compelling reason
5 participants