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

remove imports from go-cams #345

Open
3 of 4 tasks
goodb opened this issue Aug 26, 2020 · 17 comments
Open
3 of 4 tasks

remove imports from go-cams #345

goodb opened this issue Aug 26, 2020 · 17 comments
Assignees

Comments

@goodb
Copy link
Contributor

goodb commented Aug 26, 2020

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

  • Stop Minerva from adding import statements onto new or edited models
  • Ensure that OWL Object Property declarations are included with each model (as otherwise the loader can confuse them with annotation properties)
  • Do a batch update of all models on noctua-dev to remove the imports and add the declarations - test with users
  • Stop production noctua, do the batch update, restart, test.

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
Copy link
Member

kltm commented Aug 31, 2020

Should check that

  • the reasoner/validator works as expected
  • the OWL export does not contain an import statement
  • the form/ART etc. are not impacted (they shouldn't be).

@goodb
Copy link
Contributor Author

goodb commented Aug 31, 2020

@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
Copy link
Contributor Author

goodb commented Sep 16, 2020

@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
Copy link
Member

kltm commented Sep 16, 2020

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

@kltm
Copy link
Member

kltm commented Sep 17, 2020

@vanaukenk @ukemi
on noctua-dev

  • create models
  • models are coherent in form

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

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

@ukemi
Copy link

ukemi commented Sep 18, 2020

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
Copy link
Member

balhoff commented Sep 18, 2020

@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
Copy link

@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
Copy link

vanaukenk commented Sep 18, 2020

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
Copy link

ukemi commented Sep 18, 2020

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
Copy link

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
Copy link

ukemi commented Sep 18, 2020

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
Copy link

No worries @ukemi You may well be right.

@goodb
Copy link
Contributor Author

goodb commented Sep 18, 2020

@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
Copy link

@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
Copy link

From 2020-09-23 call:

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

@goodb
Copy link
Contributor Author

goodb commented Nov 11, 2020

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/

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

5 participants