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

NTR 'contains measured amount' #726

Merged
merged 20 commits into from
Nov 6, 2023
Merged

NTR 'contains measured amount' #726

merged 20 commits into from
Nov 6, 2023

Conversation

jmwhorton
Copy link
Contributor

Addresses new term request: #712 (original label was 'has actual load')

Please note: imports were added manually to other_import.owl. I couldn't see another place that made more sense. If I have added the imports incorrectly, please let me know and I'm happy to make the change.

@wdduncan
Copy link
Collaborator

@jmwhorton please adress the following:

  1. We can discuss at the next meeting, but I would prefer the label to more explicitly reflect that the range is a measurement data. E.g., "container amount measured by". "contains amount" can still be a synonym (e.g., has_exact_synonym.

  2. Change

A relation that relates a container to a measurement datum that specifies the actual amount of material in the container.

to

A relation between a container and measurement datum that specifies the actual amount of material in the container.
  1. Instead of term editor (IAO_0000117), use annotation dcterms:contributor and the editor's ORCID. Each contributor gets their own contribution annotation.
    As an example see positively correlated with. (You will have to open in Protege. OLS doesn't pull up the term.)

  2. Can you please provide an example of usage.

  3. The definition source annotation doesn't seem relevant here. "Contains" is a common word and the contributors are listed if anyone has questions.

  4. Is there an inverse relation? E.g., "measures container amount"

@nataled
Copy link
Collaborator

nataled commented Jun 13, 2023

@wdduncan I'm confused by your suggested term label change. Isn't the relation meant to be used like "this beaker 'contains amount' 400 ml"? I think the suggested change fits better with how something is being measured, like "this beaker 'container amount measured by' Mettler scale".

@wdduncan
Copy link
Collaborator

@nataled I see your point.

"this beaker 'container amount measured by' Mettler scale".

I made the suggestion b/c I was trying to make it more clear that the range is a measurement datum and not a literal.

@nataled
Copy link
Collaborator

nataled commented Jun 14, 2023

@wdduncan substituting 'as' in place of 'by' might come a bit closer (in my mind, at least) in evoking the correct range type. Even so, the 'as' version could just as likely have someone think that 'weight' or 'volume' would be an appropriate response. I'm not sure how best to do it.

@jmwhorton
Copy link
Contributor Author

I have addressed numbers 2, 3, and 5 from @wdduncan's list above. For the rest, I'll defer to the group consensus and ask @cstoeckert for an opinion.

@wdduncan
Copy link
Collaborator

@nataled I've thinking about this past few days, and I don't have any real answer regarding container amount measured by vs. container amount measured as. We can discuss at tomorrow's RO meeting. But here are some random thoughts.

  1. It would be nice if the label contains amount had a nice inverse. Not sure what this is though ... Perhaps amount in container?
  2. Normally, I'd be okay with using the label contains amount. But, there has been recent discussion about including data properties in RO. Maybe I'm worrying to much, and we should just use the label as is. If a data property for amounts in containers is proposed, we can address it then. (i.e., for now we kick the can down the road).

In any case, let's focus on wrapping this up at the meeting. This issue doesn't need to hang around too long.

src/ontology/ro-edit.owl Outdated Show resolved Hide resolved
@wdduncan
Copy link
Collaborator

Agreed on RO call to change label to contains measured amount.

src/ontology/imports/other_import.owl Outdated Show resolved Hide resolved
@wdduncan
Copy link
Collaborator

Fixes #712

@anitacaron anitacaron linked an issue Jul 18, 2023 that may be closed by this pull request
@jmwhorton
Copy link
Contributor Author

Added example of usage. I believe all issues have been addressed, and this is ready to be reviewed again.

@anitacaron anitacaron self-requested a review July 26, 2023 18:44
@wdduncan
Copy link
Collaborator

Thanks @jmwhorton
Did you manually add the domain (device) and range (measurement data item] to obi_import.owl manually?

If so, I believe those terms should be added to the obi_terms.txt file, and the import should be rebuilt. Otherwise, the changes you made to obi_import.owl will get overwritten by running ODK.

@matentzn @anitacaron can you confirm this? I can't quite recall the ODK command to rebuild and OBI import. I think it is:sh run.sh make refresh-obi. However, is it better to update the whole repo, i.e., sh run.sh make update_repo?

@wdduncan
Copy link
Collaborator

P.S. I just noticed @anitacaron comment. Not sure if it better to run sh run.sh make refresh-obi or sh run.sh make imports/obi_import.owl. I think they may do the same thing.

@jmwhorton
Copy link
Contributor Author

For the sake of clarity I'll answer both the previous commends, @wdduncan. I didn't add the imports to obi_import.owl manually. I did run make imports/obi_import.owl

However, do note that I added terms into other_import.owl manually (when I originally made the change, all of the imports were added there. I manually added them to the .owl file since other_terms.txt was empty).

Happy to make any changes if I need to.

@wdduncan
Copy link
Collaborator

Thanks @jmwhorton !
I believe the new terms should go in obi_terms.txt. Let's wait on @anitacaron or @matentzn to confirm.

@jmwhorton
Copy link
Contributor Author

To be specific, the new terms that I left in other_terms.owl are IAO terms, not OBI terms. (so I didn't want to put them in the OBI file)

@wdduncan
Copy link
Collaborator

Thanks. I didn't catch that. Ideally, we should create new IAO import, but measurement data item is in OBI ... What you think @anitacaron ?

@anitacaron
Copy link
Collaborator

You're right, @wdduncan. We need to create an IAO import and add the IAO term there instead of adding it to the other_import.owl file.

@wdduncan
Copy link
Collaborator

@anitacaron Can you modify the ODK file to create the IAO import? Also, is there documentation about the purpose of other_import.owl?

@ddooley
Copy link
Contributor

ddooley commented Jul 31, 2023

One question - can the IAO terms be imported by way of COB? I recall there was going to be some change made to COB import to enable 3rd party terms imported that way?

@wdduncan
Copy link
Collaborator

@anitacaron will be able to modify the ro-odk.yaml so that IAO is included as an import?
Also, do you have any documentation on what other_import.owl is used for?

@anitacaron
Copy link
Collaborator

anitacaron commented Aug 10, 2023

@wdduncan I think the other_imports.owl was created to have all imports, but it was manually updated. There's a comment in the file.

Annotation(rdfs:comment "Ad-hoc additional imports, manually copied. This should be split into several dynamically imported sub-modules.")

Ideally, we should remove this file. Now it only imports a few terms. It would be good to review, update, and create a proper import in another issue. For example, instead of disease (OGMS), use disease (MONDO).

Can the IAO terms be imported by way of COB?

@jmwhorton, now that we have the COB import, you can import the COB terms like OBI. Add the terms in the src/ontology/imports/cob_terms.txt file and run sh run.sh make imports/cob_import.owl
I found measurement datum - COB:0000121 and data item - IAO:0000027. But only the measurement datum is used in the new RO property, so I think you only need to add this term to the cob_terms.txt file.

src/ontology/imports/other_import.owl Outdated Show resolved Hide resolved
src/ontology/imports/other_import.owl Outdated Show resolved Hide resolved
src/ontology/imports/other_import.owl Outdated Show resolved Hide resolved
@wdduncan
Copy link
Collaborator

@jmwhorton Have had a chance to look at the changes? What is still needed to move this forward?

@jmwhorton
Copy link
Contributor Author

Requested changes have been made, @wdduncan

@anitacaron anitacaron self-requested a review August 29, 2023 12:44
@anitacaron anitacaron changed the title NTR 'contains amount' NTR 'contains measured amount' Aug 29, 2023
@jmwhorton
Copy link
Contributor Author

@anitacaron 'dc' has been replaced with 'terms' now to fit with recent changes to master.

@wdduncan
Copy link
Collaborator

wdduncan commented Nov 6, 2023

@anitacaron I don't understand which changes you requested. Have they been addressed?

@anitacaron
Copy link
Collaborator

Yes, they were addressed, and all conversations were resolved. I'm waiting for someone from the ontology to approve it, and I'll approve it after.

@wdduncan
Copy link
Collaborator

wdduncan commented Nov 6, 2023

Yes, they were addressed, and all conversations were resolved. I'm waiting for someone from the ontology to approve it, and I'll approve it after.

Ok. I approved.

@anitacaron anitacaron merged commit 047b84d into oborel:master Nov 6, 2023
1 check passed
@wdduncan
Copy link
Collaborator

wdduncan commented Nov 6, 2023

Thanks @anitacaron !!!!

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: 'contains measured amount'
5 participants