Planteome / amigo

This repo is the Planteome fork of geneontology.org AmiGO2 project. Issues in this repo should be reported only on AmiGO issues. Issues can be pushed upstream if relevant for GO.
BSD 3-Clause "New" or "Revised" License
4 stars 0 forks source link

incorrect PO links on dev #17

Closed marieALaporte closed 8 years ago

marieALaporte commented 8 years ago

PO links redirect to the old plant ontology browser on the planteome dev website (term ID column). Here an example: http://dev.planteome.org/amigo/search/ontology?q=fruit

cmungall commented 8 years ago

Hmm, the Term column seems to stay within dev, but the Term ID column goes to GO for GO IDs, presumably you want to see GO from within dev.planteome here

elserj commented 8 years ago

I've been trying to track this down and remind myself what is going on here.

I think we added the "Term ID" column a while back before our January release. At the time, it was just a text with the ontological term ID. Since then, @kltm did some work to allow the alternate ID column to work as it was failing on some GO terms (https://github.com/geneontology/amigo/issues/289). I was asked to make the text a URL that would go to the referential site for the termID. Since PO terms are still entered in dbxrefs.yaml as going to www.plantontology.org, this is working as expected.

However, maybe it is time to change the xref to point to planteome rather that the old PO site? This is a question for @jaiswalp and @cooperl09. If they decide to change to using planteome for PO terms, I can make the PR to fix the dbxref file.

Btw, I would recommend making this change as my understanding is that the PO site will not be updated again, and the data will be obsolete.

cooperl09 commented 8 years ago

I agree, the terms should link to the appropriate Planteome site, not to the old PO site. Larger question is do we want the GO terms to resolve to GO within dev.planteome, or to the GO AmiGO site. I prefer to link them to the appropriate Planteome site as well.

cooperl09 commented 8 years ago

The TO ids should be links as well.

elserj commented 8 years ago

The URL in the "Term" column already does what you are requesting. IIRC, you asked for the "Term ID" to link to the source for the ontology, ie geneontology, CheBi.

I believe the reason TO ids don't link out is because they don't have a dbxref at all, only TO_GIT is a dbxref.

My suggestion would be to either go back to non-URL text in this column, or change the dbxrefs.

kltm commented 8 years ago

@elserj We could have a quick call to touch bases, etc. too if it would help. Just to refresh, there are three main ways to control links and linking behaviour:

There is also some small behaviour in the amigo.yaml config file about what regexp to use to identify the "home" vs external id space for terms.

elserj commented 8 years ago

@kltm, after some discussion about this, we'd like to change it so that the "Term ID" column doesn't link out at all. We already have internal links in the "Term" column. Seems confusing to have other external links. I don't recall if I did something to make this change, or if this is something that we pulled from master, but it isn't like this on our older version of AmiGO that the live site is running. I'll look into it, but you may know off the top of your head faster than I can find it. I'll poke around, but if you have any ideas, let me know.

kltm commented 8 years ago

I would suggest a local handler that is a combination of the first two columns there, with the link aiming internally. Or, you could just write a local handler that skips making a link out. Or one that links internally for you (maybe by adding the GO to the list of internal links). There are a few options here. Start maybe with making "GO" internal, and then maybe looking at javascript/npm/amigo2-instance-data/lib/data/dispatch.js, including "dispatch_table" (modifications require a re-compile).

kltm commented 8 years ago

Or, as mentioned above, simply goose the linker to make GO terms internal to planteome. That could either be done with code or cheaped by changing the xrefs data before compile time.

cmungall commented 8 years ago

IMHO, I would just make this act like the GO amigo

http://amigo.geneontology.org/amigo/search/ontology?q=fruit

i.e. one column, internally linked.

kltm commented 8 years ago

...but where's the fun in that. IIRC, I think that @cooperl09 mentioned that curators wanted to see the IDs in this display as they are used to working with raw IDs...?

elserj commented 8 years ago

Yes, they did ask for that. I even found the commit (https://github.com/Planteome/amigo/commit/dada78e80b64fb2518ea1a6c4f44047df2ad6314) where I added it. Even mentioned that it shouldn't link anywhere. I think it did at that point and I'm having trouble figuring out what changed to make it so that it does link out now.

elserj commented 8 years ago

Whoops, wrong commit: https://github.com/Planteome/amigo/commit/dada78e80b64fb2518ea1a6c4f44047df2ad6314

cmungall commented 8 years ago

So there will be a separate config for dev and production?

It's great that PO-internal uses AmiGO so much, but I think we have to be careful here. We in planteome should be doing more to reach out to our users, and we should optimize the experience for users and not internal as much as possible

kltm commented 8 years ago

It's also possible to add an "internal" ontology view to the system for curators, although that is probably too much effort.

elserj commented 8 years ago

After some discussion with @kltm, this was fixed in commit https://github.com/Planteome/amigo/commit/7e35fc1e9031cdb9169ab8b44a72f5478ffe85ce. The file javascript/npm/amigo2-instance-data/generation-templates/dispatch.js.tmpl was the one that needed to be modified.

kltm commented 8 years ago

Just to have this on record, in case we need to search/link to it again, I'll paste in my explanation after I realized that I had completely misdiagnosed what was going on.

Whoops, sorry, it's been a while--that is the expected behavior.
Something a unintuitive is indeed going on here, starting in
amigo/install. To give the big-picture explanation first, there is a
general tension in the code between wanting to do everything cleanly in
an NPM/browserify way versus having the flexibility of configurations
being local to the site AmiGO installation. What we want is:

1) People loading the amigo NPM libraries from the web should always hit
the default AmiGO production services, unless they do something else
specifically
2) The JS code in AmiGO should compile cleanly using standard
NPM/browserify methods
3) Remote unit tests and the like should work with default configurations
4) Local unit tests and the like should work with local configurations
5) Non-GO sites should be able to freely customize their forks without
having too much overhead, including server configuration, handlers, etc.
6) Non-GO sites should have a straight-forward path to publishing their
own widget sets tied to their own sites

The first couple of items makes the rest pretty irritating. The solution
for this, for now, is to have the amigo JS NPM modules reside within the
amigo repository, with some portions of the NPM module code getting
generated by a trigger from the enclosing amigo. This way, the NPM
modules can still be pushed to NPM easily with different configurations,
even exotic ones (let's say you wanted to publish your own Planteome
widget sets); the cost is to have the local configuration copied back
into the npm module just before compile time.

Okay, that's the big-picture rationale. For your specific case, the code
to modify in the new system for the handler exists in this template:

 amigo/javascript/npm/amigo2-instance-data/generation-templates/dispatch.js.tmpl

A modification there should be sticky across compiles/installs when
initiated by the gulpfile. To put it another way, the NPM submodule
behaves as it normally would, except when being compiled from the amigo
above it, it which case a template contained in the NPM module itself
rolls over what's there in preference for the current amigo configuration.

I think that should get you where you are going. This is not as nice as
the old handlers.yaml and dispatch.js, but those were items from before
we used NPM and had our own JS "compiler" for modules, giving us a lot
more control.