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

Review new release artefacts (important, community-wide implications) #194

Closed
matentzn opened this issue Mar 7, 2019 · 18 comments
Closed
Assignees

Comments

@matentzn
Copy link
Contributor

matentzn commented Mar 7, 2019

We made a first stab add defining release artefacts that should cover all use cases community-wide. We need to (1) agree they are all that is needed and (2) they are defined correctly in terms of ROBOT commands. This functionality replaces what was previously done using OORT.

!!!!!!

Moved docs here

!!!!!

Please review these artefacts carefully, and let me know what you think relatively soon, in particular:
@cmungall @balhoff @dosumis @rctauber @jamesaoverton

@dosumis
Copy link
Contributor

dosumis commented Mar 7, 2019

Looks good to me.

@matentzn
Copy link
Contributor Author

matentzn commented Mar 7, 2019

I know @rctauber suggested to remove equivalent classes and potentially anonymous classes for one of these artefacts. @rctauber can you also review the ROBOT commands here just to be safe?

@jamesaoverton
Copy link
Contributor

Thanks for documenting this! This standardization will be a big step forward.

In general: The use cases for 'base' and 'full' are very clear to me. I do not understand the use cases for the four optional types. So while I accept that people must have good reasons for needing these, and I see the advantage in standardizing, my opinion is that we would be better off if the four optional types were eventually eliminated in favour of the two required types.

Specifically:

  1. For (2), I don't understand the remove, merge, merge. Why isn't one merge good enough? Are some of $(IMPORT_OWL_FILES) and $(OTHER_SRC) not already owl:imports?
  2. Also for (2), I always reason after merge to benefit from the 'foreign' axioms. I think we need to have consensus on this.
  3. For (1) it bugs me that $(OTHER_SRC) is required, since it means the information about which imports are 'internal' and 'external' is in the Makefile and not in the ontology file. But I don't have a better solution.

@matentzn
Copy link
Contributor Author

matentzn commented Mar 7, 2019

Thanks for the input @jamesoverton

  • agreed 100% on your comment on base and full. Its just like battling against a tsunami :P But there are too many groups that insist on one of the others; not providing simply hampers rolling out the ODK. I am evangelising the use of the full/base artefacts wherever people listen! Ask @dosumis he wants to keep some of them!

Specific feedback:

  1. Oops I guess you are right. For (1) base we could not do this, but yes, since we merge EVERYTHING, we can do it. Can you check the command again?
  2. Wait, reason does NOT take into account the imports? I thought it did? I thought reason merge results in exactly the identical output to merge reason! Is this not the case? In any case, I changed the order, but would still like to know. Thanks!
  3. Yeah. Good enough :P

@jamesaoverton
Copy link
Contributor

  1. Should be merge --input $< \ reason--reasoner ELK \
  2. You're right, my mistake. I agree that reason-merge should be the same as merge-reason, I just prefer the latter.

@matentzn
Copy link
Contributor Author

matentzn commented Mar 7, 2019

Oops, thanks! :)

@beckyjackson
Copy link
Collaborator

@matentzn For DO, we remove anonymous parents and equivalence axioms from our non-classified version:

$(ROBOT) remove --input $< --select imports --trim true \
remove --select "parents equivalents" --select anonymous \
annotate --ontology-iri "$(OBO)doid/$(notdir $@)"\
 --version-iri "$(OBO)doid/releases/$(DATE)/$(notdir $@)"\
 --output $@

@jamesaoverton - I know the use case for the non-classified version of DO is to have a single-inheritance hierarchy with no extra logical axioms. A lot of the OBO users rely on this version of DO. I'm not sure about the simple-non-classified version, though.

I just made a PR for selecting entities by IRI patterns in ROBOT (ontodev/robot#448) so that might be useful for generating the "simple" products.

@matentzn
Copy link
Contributor Author

matentzn commented Mar 7, 2019

thanks @rctauber that is great. @dosumis what do you think about @rctauber suggestion regarding 'non-classified'? or is this more something that goes in one of the other artefacts in your understanding of the terms?

Also, the remove trim true is redundant in your DO example right? I need to ask you something else. Gitter :P

@beckyjackson
Copy link
Collaborator

Yeah, the --trim true is redundant. We used to have --trim default to false, but we realized that most of the use cases used --trim true. :)

@jamesaoverton
Copy link
Contributor

See also #175

@dosumis
Copy link
Contributor

dosumis commented Apr 30, 2019

Hi all - I've edited descriptions of -simple and -basic (in the text at the top of this ticket), to make it clear what they are used for. Please review. The -simple use case to be required as long as people use OBO files for graph-oriented approaches to ontology use. These are core use-cases for many ontologies and include a whole zoo of term enrichment tools for GO, standard use of ontlogies by MODs as well as many tools build to work with HPO. So - I suspect we need to support these for the forseeable future.

@cmungall
Copy link
Contributor

Very clear. I suggest changing the variable name from KEEPRELATIONS to something like BASIC_RELATIONS, and having the parameter in the yaml be named analogously

@dosumis
Copy link
Contributor

dosumis commented May 13, 2019

@matentzn For DO, we remove anonymous parents and equivalence axioms from our non-classified version:

This is a very different use of non-classified than the standard OORT products produced by a number of ontologies for the past 10 years.

@jamesaoverton
Copy link
Contributor

To move forward and finish implementing this, it would be very helpful to have concrete examples. Is there a project that's building all five artifacts according to this spec, that we can use as a known-good reference?

Even better would be five smallish OWL files that we can use for integration tests.

@matentzn
Copy link
Contributor Author

For me, the number 1 reason to use ROBOT is transparency; it allows me to define transformation operations in a way that is communicable, negotiable and reviewable. I can debug and tell people: "Oh, you dont see class X because it is removed here at step 3, because you dont include it in the seed." The reason why I want to move away from owltools as far as possible is because of its obscure commands, like "make-simple". I dont think I would like to use robot release simple; it will be hard to debug, hard to communicate. This is, imho, the domain of ODK: capturing community consensus (or at least OBO consensus) on what a good release should look like. The makefile makes this explicit, tangible, and everyone can look and overwrite the bits that don't work for them.

The other thing is: finding good examples :) I would like that too, but even here, there does not seem to be anything 100% clear, especially when looking at simple and basic. Release artefact 6: simple-non-classified has now grounds in practice at all, other than HP and MP saying "I want it".

I think its best we iterate a few months over the different chains and nail them down here before we start thinking of implementing robot shortcuts, but this is just my view. Is there a concrete user story for these short cuts?

@jamesaoverton
Copy link
Contributor

I agree that we aren't ready for ROBOT shortcuts yet. I think it will make sense eventually, but I'm not in a rush. My point about shortcuts is just that it doesn't bother me if we have long ROBOT command strings for building these artifacts -- we can shorten them later.

I'd like examples, even if they aren't perfect, so that we can iterate and improve on them. Without examples it's hard to see the problems and communicate clearly.

@cmungall
Copy link
Contributor

Will comment more later, wanted to point out overlap with this ontology metadata ticket information-artifact-ontology/ontology-metadata#36 (I would like to replace these from IAO to OM IDs)

@matentzn
Copy link
Contributor Author

matentzn commented Feb 5, 2022

Lets close this as it seems the discussion subsided, and reopen when questions arise.

@matentzn matentzn closed this as completed Feb 5, 2022
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

No branches or pull requests

6 participants