Skip to content
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

add process to domain to range of concretizes #738

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

wdduncan
Copy link
Collaborator

  • add process (BFO_0000015) to domain of concretizes
  • update definition of concretizes to include process
  • add process (BFO_0000015) to range of is concretized as
  • update definition of is concretized as to include process

Fixes #650

I added process to domains and ranges in the core.owl file. Let me know if I was supposed to update ro-edit.owl instead.

@shawntanzk
Copy link
Collaborator

I added process to domains and ranges in the core.owl file. Let me know if I was supposed to update ro-edit.owl instead.

I might not be the right person to review this as I do not know where the components/core.owl derives from (I see that it is a custom component but has no makepoint in ro.makefile) but I do realise that those terms are not in ro-edit.owl (as in they are pulled in my importing component). I guess one can add those triples in edit as just dangling triples, but that would be a bit ugly in the edit owl file, so I would like you assume adding it to core is right, but again, cant figure out where core.owl came from.

PS is there a reason for all the other additions/changes in components/core.owl? (eg addition of all the BFO terms, and rdf:description to owl:class)

@wdduncan
Copy link
Collaborator Author

@anitacaron I still need two reviews to approve.

Copy link
Contributor

This PR has not seen any activity in 90 days and has been marked as stale. If it is no longer needed, please close the PR. Otherwise, please update the PR with a status update.

@github-actions github-actions bot added the stale label Nov 15, 2023
@github-actions github-actions bot removed the stale label Nov 16, 2023
@jhpoelen
Copy link
Contributor

fyi @cmungall @shawntanzk please consider reviewing @wdduncan 's pull request related to "add process to domain to range of concretizes"

@shawntanzk
Copy link
Collaborator

shawntanzk commented Apr 4, 2024

@jhpoelen sorry this slipped out of my radar when I changed job. As with my previous comment, I am not sure I am qualified enough to review this PR. Would need replies to my questions above before I can make a call. Thanks

Would probably need a bit more indepth look into why QC is failing too, but I think that should come after understanding the PR better

@jhpoelen
Copy link
Contributor

jhpoelen commented Apr 4, 2024

@shawntanzk thanks for taking the time to respond and for sharing your notes.

@wdduncan can you please review the failed QC for this pull request? Also, please nominate other reviewers as hinted in @shawntanzk earlier comment.

@wdduncan
Copy link
Collaborator Author

wdduncan commented Apr 4, 2024

@shawntanzk thanks for taking the time to respond and for sharing your notes.

@wdduncan can you please review the failed QC for this pull request? Also, please nominate other reviewers as hinted in @shawntanzk earlier comment.

@jhpoelen There is some issue going on with components/core.owl. When I tried to open the file locally, I got the error:

org.xml.sax.SAXParseException; systemId: file:/Users/duncanw/repos/OBO/obo-relations/src/ontology/components/core.owl; lineNumber: 14; columnNumber: 45; Attribute "xmlns:terms" was already specified for element "rdf:RDF".

I copied the core.owl from the master branch, and now core.owl opens w/o any issues. git diff did not return any changes. So, I will push core.owl, although I don't know what changed.

@wdduncan wdduncan requested review from balhoff and ddooley April 4, 2024 20:32
@wdduncan
Copy link
Collaborator Author

wdduncan commented Apr 4, 2024

I pinged @balhoff and @ddooley as reviewers.

@shawntanzk
Copy link
Collaborator

shawntanzk commented Apr 5, 2024

Looks like theres no diff now lol - pulling this in would make no difference >.<
@wdduncan I think the changes you made were in core.owl so when you brought it over from master, it removed all the changes. I see that in the branch now, there isn't the domain and ranges that you added, and the definition is back to the old one.
I have pushed a reverted version of core.owl - please do check that it is the changes you want. Thanks

PS reverted it back to where @anitacaron pushed master into this branch before @jhpoelen commented. Might need updating >.<

@anitacaron anitacaron removed the request for review from shawntanzk April 5, 2024 09:12
@wdduncan
Copy link
Collaborator Author

wdduncan commented Apr 5, 2024

Thanks @shawntanzk !
There was duplicate xmlns:terms="http://purl.org/dc/terms/" in the rdfs/xml file for some reason. I've deleted it.

Copy link
Contributor

github-actions bot commented Jul 5, 2024

This PR has not seen any activity in 90 days and has been marked as stale. If it is no longer needed, please close the PR. Otherwise, please update the PR with a status update.

@github-actions github-actions bot added the stale label Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add process to domain to range of concretizes
4 participants