geneontology / minerva

BSD 3-Clause "New" or "Revised" License
6 stars 8 forks source link

remove imports from go-cams #345

Open goodb opened 3 years ago

goodb commented 3 years ago

Following discussion with @balhoff , we concluded that we should:

The reasoning underlying this move:

Minerva handles reasoning over models using an independent, system wide configuration controlled globally as two start up parameters (1 ontology (for Arachne) and 2 ontology journal (for tbox queries and labels)). Having ontology imports declared in models can conflict with the system-level ontologies, causing confusion. By leaving them out, the user of the go-cam can decide what ontologies to import and when. e.g. this allows users to open go-cams in protege without editing a catalogue file. If users want to access to missing tbox information, they can always import the ontologies.

kltm commented 3 years ago

Should check that

goodb commented 3 years ago

@vanaukenk @ukemi could you take some time with models on noctua-dev to verify that all is good? I just made a change under the hood and want to make sure the engine is still running.. Thanks.

goodb commented 3 years ago

@kltm everything seems to be good to go with this change on dev. Do you want to make the change to master so we can make everything consistent ?

kltm commented 3 years ago

We'll need to coordinate time with @vanaukenk to stop the system, get the PR/update in, restart.

kltm commented 3 years ago

@vanaukenk @ukemi on noctua-dev

(- [ ] models are coherent in graph editor) (- [ ] make sure that reasoner works)

Issues, if there are any, would likely be quite serious.

ukemi commented 3 years ago

Created basic complete model in graph editor that includes MF-BP-CC-CL-EMAPA. gomodel:5f4d6ed700000063 looks good and reasoner runs without crashing. Able to add and delete individuals. Used add annoton and add individuals. PASS.

Created model in graph resulting in inference of regulation process. gomodel:5f4d6ed700000077. Inference is correct. Tested clone evidence. Model looks good. PASS

Created model in graph using BP only. gomodel:5f4d6ed700000089. Could not use EMAPA:17412 in occurs_in statement from the 'ADD Process' menu. It appears that EMAPA is missing from the autocomplete from the 'Add process' menu. Although it is available from the 'Add Individual' menu. Should 'Add Process' occurs_in be restricted to Cellular Component? FAIL

Created individuals with UBERON, EMAPA, ZFIN, XAO and WBbt anatomical structures. gomodel:5f4d6ed700000089. Correct inferences were generated from UBERON and WBbt, but not ZFIN, EMAPA or XAO. See the intestine terms and heart terms. FAIL

I suspect the failures are due to lack of bridging axioms (@balhoff )? Is there anything else I should test?

balhoff commented 3 years ago

@ukemi it might be useful to chat on Skype about the anatomy bridging you were expecting. I thought those were set up so we might need something more thorough.

vanaukenk commented 3 years ago

@ukemi @balhoff There are some other anatomy- and cell-related issues that we need to discuss. Would it be possible to set up a call to review them and decide what work needs to be done? We could also use the GO-CAM specs call time slot.

vanaukenk commented 3 years ago

Testing in the form:

I am able to create a model with extensions that renders in the form and the graph.

@tmushayahama One thing that is not working properly in the form on noctua-dev is 'Remove evidence' . This now deletes all of the evidence boxes instead of just the values within them. This does not happen on production.

Based on our discussions yesterday, I don't think this would be a result of the minerva changes, but would like to confirm what will happen when we move to production.

ukemi commented 3 years ago

Either one is good for me. I thought the EMAPA inferences used to work. But I figured that since I was testing, I would run through a bunch.

vanaukenk commented 3 years ago

I have questions about the C. elegans cell and anatomy inferences, too, and there are still some issues with typing cell and anatomical entities for the purposes of the form and table.

We're off-topic a bit for this ticket, I suspect, but let's try to schedule a time when we can discuss this with all necessary people on board.

ukemi commented 3 years ago

Sorry @vanaukenk. I wasn't sure exactly what these changes would affect. I figured that sine the ticket was called remove imports I should try using imported ontologies.

vanaukenk commented 3 years ago

No worries @ukemi You may well be right.

goodb commented 3 years ago

@ukemi @vanaukenk whether terms appear in certain type-aheads is a separate issue from the minerva changes as the type aheads are controlled via the javascript and the SOLR service - they don't touch minerva.

For the anatomy inferences, it would be helpful to know specifically what you were expecting and what actually happened. It would also be useful to see if the same thing happened on production versus master. What are the correct inferences that you are not seeing?

For clarity, the 'remove imports' thing here does NOT mean that we should lose access to any terms or inferences within the noctua/minerva system. It is a change internally regarding how Minerva specifically handles the binding of ontology term definitions to instances in the models.

It seems that we have made the mistake here of changing a lot of things at the same time.. It would have been nice to test the UI code changes independently of the server side changes. I suspect there are probably also ontology changes. So yeah.. an automated test suite that ran on top of the UI so the entire stack could be tested at once would be very awesome - perhaps it could be called necessary - and necessary to execute every time there is a merge to dev for any of the client or server code bases.

vanaukenk commented 3 years ago

@ukemi @balhoff and I discussed this morning some of the inference issues we're seeing and don't believe this relates to the minerva changes on this ticket.

Wrt the UI changes, is it possible to roll those back so we can just test any possible impact due to the minerva changes?

+1 for the automated test suite.

vanaukenk commented 3 years ago

From 2020-09-23 call:

@tmushayahama will investigate the behavior with 'Remove evidence'; appears to be on the client side.

goodb commented 3 years ago

The no imports noctua stack has been working fine for a while now.
The only thing left to do to close this ticket is to clean out existing imports. this can be done using the minerva-cli by: minerva-cli.sh --clean-gocams -i /dirofttlswithimports/ -o /dirofttlswithoutimports/