-
Notifications
You must be signed in to change notification settings - Fork 6
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
Stoichiometry not taken into account for complexes #199
Comments
@colin-combe Was that intentional, or is it a blind spot ? |
actually, i thought that worked as you would expect and it handled this correctly. |
Thanks a lot Colin ! |
We noticed that sometimes it works, like on the complex https://www.ebi.ac.uk/complexportal/complex/CPX-5684, but that is because for some reason, the participant is duplicated, rather than having a stoichiometry of 2. |
thanks, maybe that's why i thought it worked, or maybe i assumed it would always get the data like this |
Hi guys, still loitering here :) It's true, the subcomplex stoichiometry doesn't show in the viz. Not sure why but both examples are my "mine" ;-) And I can tell you the difference: Over to you for figuring out why you don't extrapolate the stoichiometry for the unlinked complex. Btw, nice addition to expand the subcomplex in the supercomplex view, much more human-friendly :) Birgit |
Hi @bmeldal ! Thanks for the explanations, that makes sense. We just need to make it work with stoichiometry too now I guess ^^ If you're speaking about the legend being clickable, yes, that's a nice little touc introduced by @jmedinaebi 😄 |
Yes, I was referring to the legend being expandable. Before, you had to open the subcomplex in a new page to see the participants. |
@EliotRagueneau - doing archaelogy on my own code... it looks like i started trying to fix this but never finished. However the commit is called "towards fixing multiple instances of same complex", implying it never arrived at the fix. Also, it clearly can't work because it doesn't reference the stoichiometry (it only mentions it in an incorrect comment). @bmeldal - hi Birgit |
Hi @colin-combe :) |
that code in https://github.com/MICommunity/ComplexViewer/blob/master/src/js/clone-complex-interactors.js is actually doing something else... |
you can see this apparently working now at https://complexviewer.org/ The code is quite confusing though. There's really three steps in the 'expansion' process:
For CPX-5684 & CPX-5605, the situation is a bit difficult to describe - the complexes that appear twice, appear twice as participants with stoichiometry one; but they aren't actually duplicated in the JSON, rather there is two separate references to the same complex (which is described once in the JSON). It means we still need to clone them and their participants to make them appear twice in CompexViewer (step 2 above). I guess what it's doing with these is giving the correct results? Though this looks like it works for the current examples, i see something strange in the code for step 2 and i think it might not work if there were more than two references to same complex. So, I'm thinking to test / look at this a bit more. Does anyone know of an example like CPX-5684/CPX-5605 (multiple references to same complex, each with stoichiometry one) where there are more than two references to the complex? |
I checked in the database and the only complexes having 2 participants which are actually the same, like for CPX-5684, are the following:
All of those examples are only involving 2 instances of the sub complex, it looks like there are no complexes more duplicated components like this. CPX-5602 doesn't seem to fit in that category, and the rendering looks correct to me. |
yes, I'll do that. I'll get back to you before end of week. |
viewer fails on https://www.ebi.ac.uk/complexportal/complex/CPX-7083 |
yes, the whole time is was saying 'CPX-5602' above, i meant CPX-5605. Which does fall into that category i think. |
CPX-7083 is failing because of the so called 'step 2' above |
This one is a bit more tricky as the 2 similar complexes are not at the same level of nesting, but I think the rendering is eactly what we expect there, unless maybe that they could share the same background |
this specific issue (not expanding complex stoich in CPX-1924) should be fixed by #201 CPX-7083 is still broken, will make seperate issue for that
i think the share same background colour suggestion is good, that makes more sense (and works better for your legend). That's changed by #201 also |
Awesome! Thanks a lot! |
you can see the shared colours at complexviewer.org |
e.g. in https://www.ebi.ac.uk/complexportal/complex/CPX-1924, we have subcomplexes with a stoichiometry, but that stoichiometry is not reflected in the visualisation. Probably need to work on
ComplexViewer/src/js/expand.js
Lines 28 to 65 in 3fda502
The text was updated successfully, but these errors were encountered: