WorldModelers / Ontologies

Ontologies for the World Modelers system
Creative Commons Attribution 4.0 International
6 stars 11 forks source link

Added Metadata in Ontology #124

Closed spilioeve closed 3 years ago

BeckySharp commented 3 years ago

looks great to me! @spilioeve would you mind uploading a version of the human readable with the new version number? (the content should be the same) @chanys note that the version # upped to 2.2

spilioeve commented 3 years ago

Hi Becky,

Which file is the human readable? I only modified the CompositionalOntology_v2.2_metadata.yml

On Wed, May 19, 2021 at 6:28 PM Becky Sharp @.***> wrote:

looks great to me! @spilioeve https://github.com/spilioeve would you mind uploading a version of the human readable with the new version number? (the content should be the same) @chanys https://github.com/chanys note that the version # upped to 2.2

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WorldModelers/Ontologies/pull/124#issuecomment-844539683, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HUAGBCBBKEQ2DNVBD3TOQ3RZANCNFSM45FMTUWQ .

BeckySharp commented 3 years ago

Hi Becky, Which file is the human readable? I only modified the CompositionalOntology_v2.2_metadata.yml On Wed, May 19, 2021 at 6:28 PM Becky Sharp @.***> wrote: looks great to me! @spilioeve https://github.com/spilioeve would you mind uploading a version of the human readable with the new version number? (the content should be the same) @chanys https://github.com/chanys note that the version # upped to 2.2 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#124 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HUAGBCBBKEQ2DNVBD3TOQ3RZANCNFSM45FMTUWQ .

Yes, and you didn't change the structure, so it should be the case that you can just rename CompositionalOntology_v2.1.yml as CompositionalOntology_v2.2.yml (the file w/o metadata is the human-readable version). The purpose of doing this would be so that the version numbers match in the 2 formats.

spilioeve commented 3 years ago

Sure, done!

On Thu, May 20, 2021 at 2:06 PM Becky Sharp @.***> wrote:

Hi Becky, Which file is the human readable? I only modified the CompositionalOntology_v2.2_metadata.yml … <#m7464606849789405217> On Wed, May 19, 2021 at 6:28 PM Becky Sharp @.***> wrote: looks great to me! @spilioeve https://github.com/spilioeve https://github.com/spilioeve would you mind uploading a version of the human readable with the new version number? (the content should be the same) @chanys https://github.com/chanys https://github.com/chanys note that the version # upped to 2.2 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#124 (comment) https://github.com/WorldModelers/Ontologies/pull/124#issuecomment-844539683>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HUAGBCBBKEQ2DNVBD3TOQ3RZANCNFSM45FMTUWQ .

Yes, and you didn't change the structure, so it should be the case that you can just rename CompositionalOntology_v2.1.yml as CompositionalOntology_v2.2.yml (the file w/o metadata is the human-readable version)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WorldModelers/Ontologies/pull/124#issuecomment-845348932, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HRJPIQ3ZPNLZHAR7TLTOVFTZANCNFSM45FMTUWQ .

BeckySharp commented 3 years ago

cool! thanks @spilioeve ! @chanys -- thoughts? @kwalcock we need to point at the new version as soon as this is merged.

BeckySharp commented 3 years ago

sorry @spilioeve I think one more thing. I think that the reason the tests are failing is that the version you added needs to be changed here too: https://github.com/WorldModelers/Ontologies/blob/master/build.sbt#L18 (am I right, @kwalcock ?) Another option would be to revert the filenames and not worry about the version numbers (which we have been kinda not updating regularly anyway)

spilioeve commented 3 years ago

Ok sounds good. I reverted the filenames to their original. Can you check if now everything is ok?

On Mon, May 24, 2021 at 12:26 PM Becky Sharp @.***> wrote:

sorry @spilioeve https://github.com/spilioeve I think one more thing. I think that the reason the tests are failing is that the version you added needs to be changed here too: https://github.com/WorldModelers/Ontologies/blob/master/build.sbt#L18 (am I right, @kwalcock https://github.com/kwalcock ?) Another option would be to revert the filenames and not worry about the version numbers (which we have been kinda not updating regularly anyway)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WorldModelers/Ontologies/pull/124#issuecomment-847162701, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HXYHSGKQLYSVWIA3JTTPJ44JANCNFSM45FMTUWQ .

bgyori commented 3 years ago

Hello, is the removal of the - wm: prefix in the ontology on purpose? This would make a big difference for us since wm is part of each grounding we use across all our existing content, filter implementations, tests etc - so while a small change this would be a significant breaking change.

kwalcock commented 3 years ago

Yes, @BeckySharp, I believe you are correct. The build file doesn't know which of the ontology files are of interest, so it needs a hint. That hint could include an updated version number, but then programs like Eidos would need to be updated to match. If need be, the versioned (for humans) name could be mapped to some standard name (for computers) in the build file.

kwalcock commented 3 years ago

The name change and the fork, if that's what it is, make it difficult for people to follow the changes (from what I can see). It might be worthwhile to finagle a PR in which that isn't the case.

spilioeve commented 3 years ago

Hi Ben,

Thanks for noticing, I just added the wm root. Keith, please lmk if this now works

Best, Eva

On Mon, May 24, 2021 at 12:37 PM Benjamin M. Gyori @.***> wrote:

Hello, is the removal of the - wm: prefix in the ontology on purpose? This would make a big difference for us since wm is part of each grounding we use across all our existing content, filter implementations, tests etc - so while a small change this would be a significant breaking change.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WorldModelers/Ontologies/pull/124#issuecomment-847181321, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HWETGLLMWYWFQNJBTDTPJ6D3ANCNFSM45FMTUWQ .

kwalcock commented 3 years ago

@spilioeve, it may be that -wm: needs a space like - wm:. The parser doesn't seem to like it.

spilioeve commented 3 years ago

Sure, fixed it

On Mon, May 24, 2021 at 1:07 PM Keith Alcock @.***> wrote:

@spilioeve https://github.com/spilioeve, it may be that -wm: needs a space like - wm:. The parser doesn't seem to like it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WorldModelers/Ontologies/pull/124#issuecomment-847199032, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGMZ3HXTAUXBDBDGX64W3HDTPKBUHANCNFSM45FMTUWQ .

kwalcock commented 3 years ago

It's working, but if nobody minds, I'm going to try to merge it into a branch first and get a pretty diff before merging to master.

BeckySharp commented 3 years ago

It's working, but if nobody minds, I'm going to try to merge it into a branch first and get a pretty diff before merging to master.

👍 but if what you said here has action items for us, please let us know! thanks :)

chanys commented 3 years ago

Thanks, I had a very brief look and it seems most of the changes have to do with adding semantic_type which could be "event" or "entity" (how are folks using this?), and revision of the node definitions.

kwalcock commented 3 years ago

There are entity, event, and property. Please see #125. A couple of indentation problems were fixed on nodes for economy and adult.

kwalcock commented 3 years ago

So, if it is not overstepping, I will close this request without merging in favor of #125.

bgyori commented 3 years ago

@kwalcock in principle you should be able to push directly to Eva's branch (it's something maintainers can do for open PRs) to add / make changes to this PR. Then it doesn't need to closed and transferred to a different PR. I might misunderstand the situation but wanted to mention this in case it's helpful.

kwalcock commented 3 years ago

I think it's a fork rather than a branch. This may be reopened soon if the other version doesn't work.

bgyori commented 3 years ago

Yes, you can push to a branch from a forked repository if a PR is opened from that branch. https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/proposing-changes-to-your-work-with-pull-requests/committing-changes-to-a-pull-request-branch-created-from-a-fork

bgyori commented 3 years ago

It is definitely an issue though that the branch was opened from spilioeve:master, it should have ideally been from a branch. So that would preclude that... sorry