Open balhoff opened 3 years ago
This looks fine to me.
@vanaukenk I want to confirm with you also.
The ShEx edit is fine, but we need to talk about how to programmatically update existing models when we make changes like this. I'd ask we not merge this until we have a plan for that in place.
Tagging a bunch of people who should be in the loop about this. @kltm @lpalbou @balhoff @ukemi @pgaudet @tmushayahama @cmungall
@vanaukenk To propagate a change like this we'd have to do something like:
If we go through dev first (recommended), we run through the above twice. If we trust it (not that big a deal since everything is in GH), once for prod and then at some point have some sync with dev so we don't confuse ourselves.
@balhoff I think there used to be an instruction set and a place that we kept our previous "migrations"?
Ideally, at some point we'd put a little money towards making this a little easier on ourselves and make a more formal built-in migration system.
Is there a check anywhere for obsolete terms (ontology/relations/etc?)
@pgaudet Besides the occasional application of ShEx, there is no periodic qc/qa on the models, much less migration/update. This is one of the known threads of the import project that has not yet been approached, but will need to be answered before completion.
Couple of notes, also related to https://github.com/geneontology/go-shapes/issues/255:
for production models, the impact will be limited: out of 3000+ models, 654 have at least one causal relationship and about 15 models currently use those relationships
for development models, the situation is very different: 682 models use directly_positively_regulates, 103 use directly_negatively_regulates, and 2 use directly_regulates. A number of those models point to Reactome pathways
for software, this is not a simple push, such a change should be discussed with the architect. A number of tools have hardcoded values about those relationships (e.g. how to fold facts into activity based on those relationships) but I believe the effect should be moderate. I am concerned however about Reactome and all Biopax conversion which will probably break at least the validation step, and this leads to my next comment
when such changes are desired, it's not just sufficient to remove relationships. One has to provide a plan of action, for instance:
on the biological / modeling aspect, we wanted to differentiate regulations by positive/negative/unknown and whether the regulation was direct or indirect (Tremayne tool to help choose relationships was based on those principles too). I think those were good principles, so I am unsure where this decision is leading. Such ticket would probably benefit to mention the intent of such change.
For the Reactome models, @dustine32 should be able to modify the import script to change those relations in the next import. So those can be fixed 'upstream'.
@lpalbou you are correct. Pure removal is always dangerous, but in this case, I checked the PR from @balhoff and the other 'regulates' relations are still there in the Shex, so I think we will be ok. Although you are correct, any models that use the removed ones will fail.
Trying to summarize:
It sounds like we need a procedure for continuous qa/qc/validation of models, but this should be the subject of another ticket
Fixes #255.