Skip to content

New property-based time model. Fixes #499 #619

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

Closed
wants to merge 35 commits into from

Conversation

rjyounes
Copy link
Collaborator

No description provided.

@rjyounes
Copy link
Collaborator Author

Anyone who wants to review in addition to the assigned reviewers, please do!

I've added a few skos:editorialNotes to weigh in on.

@rjyounes rjyounes marked this pull request as draft January 27, 2022 23:23
@rjyounes rjyounes marked this pull request as ready for review January 27, 2022 23:24
Copy link
Contributor

@mkumba mkumba left a comment

Choose a reason for hiding this comment

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

I'm having a heck of a time figuring out how and where to put comments, hope this is it.

I'm good with changes. In terms of the editorial notes that were in the presentation and didn't make the file, yeah I think that's a good idea.

Also on the question of atDateTime being just gist:dateTime I'm ok with that.

@rjyounes
Copy link
Collaborator Author

@mkumba @marksem @DanCarey404: @uscholdm and I both think CurrentEvent is preferable to and clearer than ContemporaryEvent, and I've made that update to the file. Please comment if you don't like it.

@rjyounes rjyounes changed the title Clean up annotations and local names in Dave's new time model New property-based time model Jan 28, 2022
@rjyounes rjyounes changed the title New property-based time model New property-based time model. Fixes #499 Jan 28, 2022
@rjyounes rjyounes mentioned this pull request Jan 28, 2022
@uscholdm
Copy link
Contributor

I suggest using "OngoingEvent" because a very recent event will generally be regarded a current. The term 'current event' refers to a general time period not whether the event is still happening. E.g. a trial that was just completed, its over but it's in the news and is regarded as a current event.

@rjyounes
Copy link
Collaborator Author

@uscholdm I think I agree with you. However, does "ongoing" lend a sense of continuing over some significant period of time? E.g., I am currently stopped at a red light, but I wouldn't call that an ongoing event. Is this too much of a quibble?

@rjyounes
Copy link
Collaborator Author

I think the advantage of Contemporary is it contrasts more naturally with Historical. But I'm OK with Contemporary, Ongoing, or Current, whatever others prefer.

@uscholdm
Copy link
Contributor

Let's hope the event of discussing this is not ongoing for too much longer :-)

@rjyounes rjyounes force-pushed the feature/issue-499-time-properties-ryounes branch from 79b44e5 to 7c62a39 Compare January 28, 2022 18:24
@rjyounes
Copy link
Collaborator Author

rjyounes commented Feb 2, 2022

Question: If the date predicates have precision +/- 12 hours, shouldn't the minute predicates have precision +/- 30 seconds rather than 1 minute?

@bpelakh
Copy link
Contributor

bpelakh commented Feb 3, 2022

Just a voice from the peanut gallery, but it's about time (heh) that this change was made. 👍

@rjyounes
Copy link
Collaborator Author

rjyounes commented Feb 3, 2022

@bpelakh Since this is a public repository, you would have been welcome to submit a PR!

@DanCarey404
Copy link
Contributor

I went with the term "contemporary" because I had an essentially boolean category :Contemporaneity. And I had that because there was no real word for "currentness". I prefer contemporary or current, but not ongoing.

@DanCarey404
Copy link
Contributor

I used :atDateTime versus just :dateTime because the latter looks too much like a datatype, in my opinion. Also, the subproperties I have created in my projects feel more natural with the "at" included. :startAtDateTime, validateAtDateTime, changeAtDateTime, etc.

@rjyounes
Copy link
Collaborator Author

rjyounes commented Feb 4, 2022

@DanCarey404 Regarding the property names, datatype property names are nouns by convention, and startDateTime is a noun, whereas startAtDateTime is verbal. It's also less awkward, IMO. I had the same concern about gist:dateTime but Dave thought it was fine, and I wanted to keep the names parallel. I'd be OK with gist:atDateTime and gist:startDateTime, etc., though.

@DanCarey404
Copy link
Contributor

This property name is definitely not a hill to die on. I just wanted to explain the origin in case anyone found it persuasive.

Copy link
Contributor

@uscholdm uscholdm left a comment

Choose a reason for hiding this comment

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

See inline. A few key things.

  • there is some stuff in this PR that belongs elsewhere - how did it get here?
  • need more details in release notes

rjyounes and others added 22 commits February 11, 2022 11:40
This property should have been removed as per issue 540. While the other changes proposed in that issue were implemented, the property itself was not removed.
…eTime to gist:atDateTime; fix some subproperty assertions
@rjyounes rjyounes force-pushed the feature/issue-499-time-properties-ryounes branch from 0855d0c to eb794a5 Compare February 11, 2022 16:46
Replace construct (subClassOf [ intersectionOf X Y]) with simpler (subClassOf X, Y). Tweak a couple of definitions.
@rjyounes rjyounes dismissed uscholdm’s stale review February 11, 2022 17:24

All requested changes have been addressed.

@rjyounes
Copy link
Collaborator Author

Closing this PR because history has gotten screwed up. Have re-opened as PR #629 with a clean history.

@rjyounes rjyounes closed this Feb 14, 2022
@rjyounes rjyounes deleted the feature/issue-499-time-properties-ryounes branch February 14, 2022 18:58
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.

6 participants