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

measures characteristic #717

Merged
merged 26 commits into from
Apr 4, 2024
Merged

measures characteristic #717

merged 26 commits into from
Apr 4, 2024

Conversation

ddooley
Copy link
Contributor

@ddooley ddooley commented May 23, 2023

and inverse 'characteristic measured by'

and inverse 'characteristic measured by'
Copy link
Collaborator

@bpeters42 bpeters42 left a comment

Choose a reason for hiding this comment

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

Diff has a bunch of lines moving around. More than expected.
Inverse property is incorrect,.

@wdduncan
Copy link
Collaborator

Since the the relation holds between a process and a quality, do we to make the label more reflective of that? E.g.:

  • process measures characteristic
  • characteristic measured by process

This would make it clear that it is not a datum that holds measurement information.

@wdduncan
Copy link
Collaborator

Since the COB import has been added to the ODK file, I think this addresses #716.
Do others agree?

@ddooley
Copy link
Contributor Author

ddooley commented May 30, 2023

Re "process measures characteristic" label etcl, I feel it is intuitive that "measures" is tied to a measuring process, so no need to spell that out in label.

@wdduncan
Copy link
Collaborator

I agree 'measures' brings to mind a process, but I thought it may need to be made clearer what is doing the measuring.
E.g., When I step on scale, there is a measurement that happens. But, just reading the label measures characteristic, one might not understand whether it is it the scale (i.e., device) that measures my weight or the applying of pressure (i.e., process) that does the measuring?
For the measures characteristic relation, it is the latter, but I can see how someone might think it is the former (i.e., device).

@bpeters42
Copy link
Collaborator

I added my comments to the issue rather than this pull request so that they can viewed in context.

Note: This should be imported via COB when that is possible.
and changing range to assay from 'planned process'
@ddooley
Copy link
Contributor Author

ddooley commented Jun 13, 2023

I'm going to have to redo this one. Its getting too complicated trying to resolve diffs with pre-UBERON integration

@anitacaron
Copy link
Collaborator

Hi @ddooley, I'll update this PR to align with the master. You don't need to worry about redoing the work.

@anitacaron anitacaron linked an issue Jul 7, 2023 that may be closed by this pull request
@wdduncan wdduncan self-requested a review July 7, 2023 13:35
wdduncan
wdduncan previously approved these changes Jul 7, 2023
@anitacaron
Copy link
Collaborator

Note: COB import is empty. If there isn't a COB term to import, I'd recommend removing the COB import.

@ddooley
Copy link
Contributor Author

ddooley commented Jul 7, 2023

Yes I guess that COB import can be removed. I'd put it in thinking I could import OBI terms that way, but to my surprise one can't fetch 3rd party terms from COB indirectly (as mentioned in last RO meeting).

@ddooley
Copy link
Contributor Author

ddooley commented Aug 10, 2023

Switch to BFO:0000020 is now done!

@ddooley
Copy link
Contributor Author

ddooley commented Aug 10, 2023

I think @bpeters42 conversation about big diff is also taken care of with your work.

Copy link
Contributor

github-actions bot commented Nov 9, 2023

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 9, 2023
@ddooley
Copy link
Contributor Author

ddooley commented Jan 22, 2024

I did another merge of master into this branch and I think outstanding issues are taken care of?

src/ontology/ro-edit.owl Outdated Show resolved Hide resolved
src/ontology/ro-edit.owl Outdated Show resolved Hide resolved
src/ontology/ro-edit.owl Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the stale label Jan 23, 2024
@ddooley
Copy link
Contributor Author

ddooley commented Feb 26, 2024

@wdduncan @bpeters42 final touchups are finished!

@anitacaron anitacaron self-requested a review March 1, 2024 17:05
anitacaron
anitacaron previously approved these changes Mar 1, 2024
Copy link
Collaborator

@anitacaron anitacaron left a comment

Choose a reason for hiding this comment

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

Technically, I approve. It still needs an ontology review.

wdduncan
wdduncan previously approved these changes Mar 25, 2024
src/ontology/ro-edit.owl Outdated Show resolved Hide resolved
@bpeters42
Copy link
Collaborator

I think the pull request as is doesn't do much harm; there is still a lot to be resolved for when we transition to modeling values of measurements via @jamesaoverton approach, but this will not make that worse, and it has been requested for a while. So I would approve. (apart from the minor typo I found)

@jamesaoverton
Copy link
Contributor

@bpeters42 I have concerns about the bigger picture here, but I agree that this PR should go ahead.

@anitacaron anitacaron dismissed stale reviews from wdduncan and themself via 6931c96 March 28, 2024 16:57
Copy link
Collaborator

@anitacaron anitacaron left a comment

Choose a reason for hiding this comment

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

Approving again after fixing a typo in the inverse property's definition.

@anitacaron anitacaron merged commit b600552 into oborel:master Apr 4, 2024
1 check passed
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.

NTR: measures characteristic
6 participants