search

INCATools / ontology-development-kit

Bootstrap an OBO Library ontology
http://incatools.github.io/ontology-development-kit/
BSD 3-Clause "New" or "Revised" License
223 stars 54 forks source link

Remove APs from merged import #927

Closed matentzn closed 7 months ago

matentzn commented 1 year ago

adds a feature to the merged import pipeline that strips away any non-declared APs from the merged import.

matentzn commented 7 months ago

@anitacaron can you carefully think this through? It looks sane, but maybe I have not thought of all the implications

anitacaron commented 7 months ago

This is working, but not sure if it's how you expect. The only annotations in the merged_import.owl are listed in the merged_terms_combined.txt.

anitacaron commented 7 months ago

Let me know if this is what you mean and I'll approve it.

matentzn commented 7 months ago

Thank you @anitacaron. Indeed, it seems work as intended (even though I am a bit surprised --signature true is not needed, but, oh well)!

Ok, now last question to both of you @anitacaron @gouttegd :

I think this is a good change. This will drastically (drastically) reduce the size of our merged imports on the one side, while making it possible to the user to add any AP they like explicitly. On the other side (actually the same point), it will remove a lot of annotation assertions. People might be wondering: why cant I see the comments of this imported class.

image

Can you both provide an approving review if you agree with the tradeoffs and with my judgement of it?

gouttegd commented 7 months ago

What’s the behaviour wrt to obsolete terms (since obsoletion is represented by an annotation property)?

Let’s say my ontology (A) imports a term B:1234 from ontology B. At some point, editors of B decides to obsolete B:1234, and they make a new release where B:1234 is now obsolete.

If owl:deprecated is not explicitly listed among the annotation properties to preserve (which it is not, by default, from what I can tell), what will happen when I refresh the B import in A? Will B:1234 correctly appear as obsolete?

anitacaron commented 7 months ago

It didn't remove owl annotations, so the obsolete terms remained unchanged.

gouttegd commented 7 months ago

OK, I’ll need to test locally, because right now I don’t understand which annotations exactly are supposed to be removed.

gouttegd commented 7 months ago

In FBbt, this removes a lot of classes from CL and GO, but they are classes that probably should not have been there to begin with (they are not referenced by anything in FBbt). I am not sure I understand why.

But the resulting import file seems good, though.

matentzn commented 7 months ago

In FBbt, this removes a lot of classes from CL and GO

This should not remove any classes..

gouttegd commented 7 months ago

My bad, made a comparison on the wrong builds. The classes that were removed were due to other changes, not this one.

None of the annotations that are removed would be missed, I think, and if needed, listing the properties to preserve in annotation_properties does work as expected.

Looks good to me.