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 #1784 merge #1804

Merged
merged 7 commits into from
Jan 30, 2023
Merged

Addresses #1784 merge #1804

merged 7 commits into from
Jan 30, 2023

Conversation

aleixpuigb
Copy link
Collaborator

Addresses #1784 merge neuroendocrine cell and neurosecretory neuron

Addresses #1784 merge neuroendocrine cell and neurosecretory neuron
@aleixpuigb aleixpuigb requested a review from a user January 18, 2023 10:10
@aleixpuigb aleixpuigb self-assigned this Jan 18, 2023
@aleixpuigb aleixpuigb linked an issue Jan 18, 2023 that may be closed by this pull request
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

There appear to be unintended changes to mappings.owl.

@anitacaron
Copy link
Contributor

@gouttegd, can you verify the changes in the components/mappings.owl, please?

@gouttegd
Copy link
Collaborator

Almost all changes to mappings.owl are formatting changes, apart from this axiom that was added:

AnnotationAssertion(<http://www.geneontology.org/formats/oboInOwl#hasDbXref> <http://purl.obolibrary.org/obo/CL_0000165> "FBbt:00005130"^^xsd:string)

Have you manually added this cross-reference to CL:0000165? If so, that’s not how cross-references to FBbt are handled. They are managed on the FBbt side, and the mappings.owl file is automatically generated from what FBbt provides.

Currently, FBbt:00005130 is mapped to CL:0000381. Since the CL term is now merged with CL:0000165, I will update the mapping to make it point to CL:0000165, and this will be automatically picked up by CL at the next import refresh.

@ghost
Copy link

ghost commented Jan 25, 2023

Have you manually added this cross-reference to CL:0000165?

@aleixpuigb, can you clarify if you manually made any changes to the mappings.owl file?
@gouttegd, thank you for checking this.

@aleixpuigb
Copy link
Collaborator Author

Thank you @anitacaron and @gouttegd for looking into this.

Have you manually added this cross-reference to CL:0000165?

I have not editted the mapping.owl file. However, Protégé didn't allow me to set the new synonym CL:0000165 'neurosecretory neuron' as xsd:string type, so I did it manually using Atom on the cl-edit.owl file.

src/ontology/cl-edit.owl Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

If you confirm line 7801 was intentionally deleted, I will approve.
Otherwise, please see other inline comments.

@gouttegd
Copy link
Collaborator

I have not edited the mapping.owl file.

OK, sorry to bother you with that but I’d like to understand how that file got modified.

One way this could have happened is if you cut the FBbt:00005130 cross-reference from the CL:0000381 class and pasted it into the CL:0000165 class (in Protégé, not by manually editing cl-edit.owl). Is that what you did?

This would explain the changes to mappings.owl because, since the cross-reference on CL:0000381 is defined in mappings.owl, when you cut-and-paste it Protégé would keep it in its original file.

@aleixpuigb
Copy link
Collaborator Author

One way this could have happened is if you cut the FBbt:00005130 cross-reference from the CL:0000381 class and pasted it into the CL:0000165 class (in Protégé, not by manually editing cl-edit.owl). Is that what you did?

I don't think I could do that because FBbt:00005130 does not appear on Protégé and it has not been modified on cl-edit.owl. I haven't done it manually either.

@gouttegd
Copy link
Collaborator

because FBbt:00005130 does not appear on Protégé

The class itself is of course not there (it belongs to FBbt), but the CL:0000381 class does have a cross-reference to it, which is visible in Protégé.

Have you followed the merging procedure described in the OBOOK? Or a similar procedure that involves duplicating the class to be obsoleted (and its annotations)? That would also explain how mappings.owl got modified.

@aleixpuigb
Copy link
Collaborator Author

Have you followed the merging procedure described in the OBOOK?

Yes, that's the procedure I followed

@gouttegd
Copy link
Collaborator

Perfect, then it’s normal that the mappings.owl file got modified (it’s a side-effect of that procedure), even though it’s unfortunate.

We need better documentation to explain to editors what to do when something like that happen (basically, that they should not commit the changes to the mappings.owl file).

Thanks for helping me sorting that out.

@aleixpuigb
Copy link
Collaborator Author

Thank you @gouttegd. I will not do it again!

@gouttegd
Copy link
Collaborator

Not your fault! There was nothing in the documentation to warn you about this problem.

@ghost
Copy link

ghost commented Jan 27, 2023

Thanks for the follow up, @gouttegd.
@aleixpuigb, after merging, can you please announce the obsoleted term in the cl-editors Slack channel?
If you update the merging procedure documentation to reflect @gouttegd's comments, you can assign me the PR to review.

@gouttegd
Copy link
Collaborator

If you update the merging procedure documentation

Not sure the OBOOK is the right place. The mappings.owl component is specific to Uberon/CL. Other ontologies don’t have have such a component – or even worse, they could have a similarly named component that is used for other purposes and that is subject to different rules.

I think what we need is some documentation directly in Uberon’s docs/ folder, describing what this component is, how it is produced, and what to do when it is inadvertently modified (then CL can simply point to the Uberon document, to avoid duplicating it).

For better or for worse, the mapping component is my thing, so I’ll take care of that.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Of note, clinicalkey.com dbxref behind a paywall.

@aleixpuigb aleixpuigb merged commit 26734f2 into master Jan 30, 2023
@aleixpuigb aleixpuigb deleted the 1784_merge_neuroendocrine branch January 30, 2023 09:17
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.

[Merge] 'neuroendocrine cell' and 'neurosecretory neuron'
3 participants