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

Addresses #1582-NTR-epithelial_cells_DCT #1644

Merged
6 commits merged into from
Jun 24, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 21, 2022

Addresses #1582-NTR-epithelial_cells_DCT

@ghost ghost self-assigned this Jun 21, 2022
@github-actions
Copy link

github-actions bot commented Jun 21, 2022

Here's a diff of how these changes impact the classified ontology (on -simple file):

Ontology comparison

Left

  • Ontology IRI: http://purl.obolibrary.org/obo/cl/cl-simple.owl
  • Version IRI: http://purl.obolibrary.org/obo/cl/releases/2022-06-23/cl-simple.owl
  • Loaded from: file:/__w/cell-ontology/cell-ontology/src/ontology/cl-simple-master.owl/cl-simple.owl

Right

  • Ontology IRI: http://purl.obolibrary.org/obo/cl/cl-simple.owl
  • Version IRI: http://purl.obolibrary.org/obo/cl/releases/2022-06-23/cl-simple.owl
  • Loaded from: file:/__w/cell-ontology/cell-ontology/src/ontology/cl-simple-pr.owl/cl-simple.owl

Ontology imports

Ontology annotations

CL_4030016 http://purl.obolibrary.org/obo/CL_4030016

Added

CL_4030017 http://purl.obolibrary.org/obo/CL_4030017

Added

serotonergic neuron http://purl.obolibrary.org/obo/CL_0000850

Added

@github-actions
Copy link

github-actions bot commented Jun 21, 2022

Here's a diff of your edit file (unreasoned)

Ontology comparison

Left

  • Ontology IRI: http://purl.obolibrary.org/obo/cl.owl
  • Version IRI: None
  • Loaded from: file:/__w/cell-ontology/cell-ontology/master/src/ontology/cl-edit.owl

Right

  • Ontology IRI: http://purl.obolibrary.org/obo/cl.owl
  • Version IRI: None
  • Loaded from: file:/__w/cell-ontology/cell-ontology/src/ontology/cl-edit.owl

Ontology imports

Ontology annotations

CL_4030016 http://purl.obolibrary.org/obo/CL_4030016

Added

CL_4030017 http://purl.obolibrary.org/obo/CL_4030017

Added

early distal convoluted tubule http://purl.obolibrary.org/obo/UBERON_0005101

Added

late distal convoluted tubule http://purl.obolibrary.org/obo/UBERON_0005102

Added

@ghost ghost requested review from emquardokus and shawntanzk June 21, 2022 14:25
@ghost ghost added this to In progress in ASCT+B_validation_review via automation Jun 21, 2022
@shawntanzk
Copy link
Contributor

Just to double - check early and late are segments and not time right? lol
I think if that is the case, it might be worth adding it in uberon and doing that part of accordingly given this cell type is defined by its locality. Also initially when looking at the definition and terms, I thought it was to do with time (eg early stage of life, late stage of life) (might also come from me coming from a developmental psychology lab, then a lab that did Alzheimers related stuff >.< ) Either way some clarity might be useful - an Uberon term with the appropriate definition would do the trick, but if you think its not useful to have in uberon, then perhaps some clarify in the defs

@ghost
Copy link
Author

ghost commented Jun 23, 2022

I think if that is the case, it might be worth adding it in uberon and doing that part of accordingly given this cell type is defined by its locality.

Thank you for the suggestion, @shawntanzk. I found that the UBERON terms already existed, but would need to be imported to CL.
This was my first attempt at an import, so I will especially appreciate you review. I see many files have changed in the process and I will have to resolve some conflicts.
FYI, @matentzn

…into 1582-NTR-epithelial-cells-of-distal-convoluted-tubule
@@ -142,6 +142,9 @@
<Class rdf:about="http://purl.obolibrary.org/obo/CL_0000381">
<oboInOwl:hasDbXref rdf:datatype="http://www.w3.org/2001/XMLSchema#string">FBbt:00005130</oboInOwl:hasDbXref>
</Class>
<Class rdf:about="http://purl.obolibrary.org/obo/CL_0000850">
<oboInOwl:hasDbXref rdf:datatype="http://www.w3.org/2001/XMLSchema#string">FBbt:00005133</oboInOwl:hasDbXref>
Copy link
Contributor

Choose a reason for hiding this comment

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

@gouttegd - could you help make sure this is correct - I think it got updated with import updates

Copy link
Collaborator

Choose a reason for hiding this comment

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

That’s correct, yes. It’s a change that I made recently in the FBbt-to-CL mappings. That it got picked up during an import update is the expected behaviour, I believe.

@shawntanzk
Copy link
Contributor

This was my first attempt at an import, so I will especially appreciate you review. I see many files have changed in the process and I will have to resolve some conflicts.

It all looks good - the file changes I'm not sure about but its not anything on you, I've asked @gouttegd to look at them to make sure they are right as its fly stuff. But basically you did everything right with imports :)

@shawntanzk shawntanzk requested a review from gouttegd June 23, 2022 12:52
@ghost
Copy link
Author

ghost commented Jun 23, 2022

@shawntanzk, I resolved 9 conflicts in the merged_import.owl file, but most were arbitrary decisions. I would not know another way to approach it and do not know why so many conflicts appeared. If we there is a way to test that I did not make any breaking changes, pls advise.

FYI, @matentzn.

Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

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

All looks great - I am a bit puzzled but they fact that the components are refreshed although you just refresh the imports. What exact command did you run in the end?

@shawntanzk
Copy link
Contributor

shawntanzk commented Jun 23, 2022

@bvarner-ebi - I think you can make equivalent classes and in fact I think you should follow exactly the the textual definition states and let the reasoner do the work (of course making sure the reasoner works, if not figuring out why). so:

epithelial cell of early distal convoluted tubule
def: Epithelial cell located in the early distal convoluted tubule.
Equivalent class: epithelial cell and ("part of" some "early distal convoluted tubule")

This should automatically place it under "kidney distal convoluted tubule epithelial cell" (if not we should look at where the modelling is wrong, or if your def needs to be edited, not sure if early distal convoluted tubule has kidney part, not sure of the biology)

PS - sorry for being annoying and not bringing this up earlier >.< just realised.

@matentzn
Copy link
Contributor

@bvarner-ebi there should never be any conflicts on imports. The only way this can happen is if you opened a new branch, and someone else came to update the import before you, and then you didn't update your branch from master first. Generally:

  1. Always update from master before running an import
  2. Then run the import

I suggest you rerun: sh run.sh make no-mirror-refresh-merged just to be sure (after updating the branch from master)

@ghost
Copy link
Author

ghost commented Jun 23, 2022

All looks great - I am a bit puzzled but they fact that the components are refreshed although you just refresh the imports. What exact command did you run in the end?

sh run.sh make PAT=false imports/go_import.owl -B (I now know this was not correct)

sh run.sh make IMP=true MIR=true PAT=false imports/go_import.owl -B (I now know this was not correct)

sh run.sh make imports/merged_import.owl

@matentzn
Copy link
Contributor

OK in the end it makes sense! All good. But I would still recommend rerunning the command i suggest above - never resolve conflicts on imports, always rerun!

Copy link
Collaborator

@gouttegd gouttegd left a comment

Choose a reason for hiding this comment

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

All changes regarding the FBbt-CL mappings are normal and expected.

@gouttegd
Copy link
Collaborator

I am a bit puzzled but they fact that the components are refreshed although you just refresh the imports.

Unless I’m missing something, I believe this is normal.

Components are listed in OTHER_SRC:

OTHER_SRC = $(PATTERNDIR)/definitions.owl $(COMPONENTSDIR)/hra_subset.owl $(COMPONENTSDIR)/mappings.owl

OTHER_SRC is a prerequisite for SRC_MERGED, which itself is a prerequisite for PRESEED and ultimately for the $(IMPORTDIR)/merged_terms_combined.txt, which is updated when imports are refreshed.

@ghost
Copy link
Author

ghost commented Jun 23, 2022

OK in the end it makes sense! All good. But I would still recommend rerunning the command i suggest above - never resolve conflicts on imports, always rerun!

Steps I'm now doing:

git checkout 1582-NTR-epithelial-cells-of-distal-convoluted-tubule
git merge master
"Already up to date."

sh run.sh make no-mirror-refresh-merged
sh run.sh make imports/merged_import.owl

@ghost
Copy link
Author

ghost commented Jun 23, 2022

@bvarner-ebi - I think you can make equivalent classes and in fact I think you should follow exactly the the textual definition states and let the reasoner do the work (of course making sure the reasoner works, if not figuring out why). so:

epithelial cell of early distal convoluted tubule def: Epithelial cell located in the early distal convoluted tubule. Equivalent class: epithelial cell and ("part of" some "early distal convoluted tubule")

This should automatically place it under "kidney distal convoluted tubule epithelial cell" (if not we should look at where the modelling is wrong, or if your def needs to be edited, not sure if early distal convoluted tubule has kidney part, not sure of the biology)

PS - sorry for being annoying and not bringing this up earlier >.< just realised.

Great suggestion. I updated that in the latest commit.

Copy link
Contributor

@shawntanzk shawntanzk 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 (just last bit of tidying up :))

src/ontology/cl-edit.owl Show resolved Hide resolved
src/ontology/cl-edit.owl Show resolved Hide resolved
@matentzn
Copy link
Contributor

sh run.sh make no-mirror-refresh-merged
sh run.sh make imports/merged_import.owl

No need: sh run.sh make no-mirror-refresh-merged is all you needed to do!

@ghost
Copy link
Author

ghost commented Jun 23, 2022

sh run.sh make no-mirror-refresh-merged
sh run.sh make imports/merged_import.owl

No need: sh run.sh make no-mirror-refresh-merged is all you needed to do!

OK. I was following the advice from Slack: "CL uses merged import so you want to run sh run.sh make imports/merged_import.owl"

https://oboacademy.github.io/obook/howto/update-import/#Refresh-imports

@ghost ghost requested a review from shawntanzk June 23, 2022 14:53
@matentzn
Copy link
Contributor

@bvarner-ebi I think in the general case, the instructions on obook are fine for the general case - sh run.sh make imports/merged_import.owl just downloads all the mirrors which seems wasteful (the no-mirror command skips downloading ontologies).

@ghost
Copy link
Author

ghost commented Jun 23, 2022

@bvarner-ebi I think in the general case, the instructions on obook are fine for the general case - sh run.sh make imports/merged_import.owl just downloads all the mirrors which seems wasteful (the no-mirror command skips downloading ontologies).

Thanks for explaining.
If there is a page explaining what mirrors are and how to check if they are updated, that would be helpful to link to the instructions.
I read "if your mirrors are updated... ", but did not know how to check if they were.

@matentzn
Copy link
Contributor

Can you make an obook issue about this I will deal with it asap

@ghost ghost merged commit c107f7f into master Jun 24, 2022
ASCT+B_validation_review automation moved this from In progress to Done Jun 24, 2022
@ghost ghost deleted the 1582-NTR-epithelial-cells-of-distal-convoluted-tubule branch June 24, 2022 10:40
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

[NTR] [ASCT+B table] epithelial cell of distal convoluted tubule (2 terms for early and late segments)
3 participants