caltechlibrary / cell-atlas

Cell atlas
https://cellstructureatlas.org
Other
5 stars 2 forks source link

Protein Viewer #12

Closed KianBadie closed 3 years ago

KianBadie commented 3 years ago

Issue to discuss integration of a protein viewer into the Cell Atlas.

KianBadie commented 3 years ago

@coiko No problem! I will go ahead and update the version then. Yeah, the lower resolution definitely does not look as good. I can tweak it some more and see if it yields good results.

And sounds good! I will go ahead and implement that then!

KianBadie commented 3 years ago

@coiko Unfortunately, I was not able to find a way to outline certain selections of the protein in PV. Do you have any other ideas? Would the coloring the selection black be a suitable choice?

coiko commented 3 years ago

@KianBadie No worries! That does sound pretty advanced now that I think about it. Black would be great, or white (since it still shows the black stroke on the sides) - I think either one would stand out nicely. Thanks!

KianBadie commented 3 years ago

@coiko Sounds good and no problem!

KianBadie commented 3 years ago

@coiko I pushed the atom label updates today. They are available to use on mobile and desktop. On desktop, I used to have the label/highlight display when you hovered over a structure. However, I think this created a performance hit as PV needed to redraw every time you wanted to highlight a part of the structure. This was fine on smaller proteins, but the lag was noticeable when you would move your mouse around. The highlight wouldn't register until the mouse was still. So I changed it to be a click to highlight functionality. If you're interested in the hovering functionality, let me know and we can test it out and see if the performance hit is acceptable (since it was not as bad as the trying to render balls and sticks on huge proteins).

coiko commented 3 years ago

Thanks @KianBadie! It looks great - I like the black highlight. I also like the idea of clicking rather than hovering to highlight a residue. I think it's good to keep the computational demands as light as possible.

KianBadie commented 3 years ago

@coiko Like I said in our meeting today, I have the functionality to limit rendering options ready to merge. However, I wanted to check to see if you wanted to take any more time to check things out before I limit any rendering options. I just wanted to make sure I wasn't limiting options before giving a chance to your devices to see if they could handle them. I'm still fielding things out, but right now I still have the threshold at 5MB and taking away just ball/sticks on desktop and ball/sticks + spheres on mobile.

If you were ok with merging whatever starting point we have now, I can merge things and we can try increasing/decreasing the file size/options from there. What do you think?

coiko commented 3 years ago

@KianBadie Thanks so much for checking, but I've gotten a chance to play around with the current options, so go ahead and merge in your awesome limiting function! And I think those parameters sound perfect.

KianBadie commented 3 years ago

@coiko No problem and sounds good! I will go ahead and merge them. And I'll actually continue the protein viewer stress test next week, as I think I am protein viewer-ed out for the day! Next week I will see if things should be increased or decreased and I'll accept the 5MBs and removing both the ball/sticks + spheres option (but keeping spheres on desktop) for now!

KianBadie commented 3 years ago

@coiko It's merged! Please let me know if you experience any issues

KianBadie commented 3 years ago

@coiko I added HOH to the look up dictionary, but please let me know if you encounter anything else that gives a wonky label in the protein viewer!

coiko commented 3 years ago

@KianBadie Perfect - will do! Thank you so much for getting this merged in! I'll have fun playing with it and let you know if I find any issues. In the meantime, have a great protein-viewer-less weekend!

KianBadie commented 3 years ago

@coiko I attached a gif below of what the new gradient looks like on a random pdb. It only applies to the "Individual Proteins" option, and the colors on the other 2 options are the same as before. I am not sure if there is a specific way the gradient is supposed to be carried throughout the protein. Does the below gif look as expected? Or is there something different from expected?

As a side note, the new color gradient is very nice and easy on the eyes.

Gif

gradient

coiko commented 3 years ago

@KianBadie Oh beautiful! It looks perfect!

KianBadie commented 3 years ago

@coiko Awesome! That is great to hear. Similarly to my above question, how does the secondary structure look?

Screenshot

ssGradient

coiko commented 3 years ago

@KianBadie It looks great! It's coloring exactly what it's supposed to color. I think it might be better, though, to reverse the blue and orange because most proteins have many more helices (currently orange) than strands (currently blue) so the orange would make the rarer element stand out. If it's easy and you don't mind trying it, could you flip the two colors?

KianBadie commented 3 years ago

@coiko Not a problem, I flipped the colors and pushed the changes to the testing site. they should be reflected their soon. Please let me know if anything needs changing or if something was implemented not as intended!

coiko commented 3 years ago

@KianBadie Thank you so much! It looks perfect!

KianBadie commented 3 years ago

@coiko I merged the protein viewer updates to the testing site. Just in case you don't remember all of them, the updates were:

Please let me know if you notice anything else!

coiko commented 3 years ago

Beautiful - thanks @KianBadie! Those changes look great! Unfortunately, I'm getting the non-loading problem again with a few of the structures (most noticeably the ones in Chapter 2). Here's the error (from Chrome):

Screen Shot 2021-08-09 at 1 12 39 PM

Also, on an unrelated (except related to the protein viewer) note, I noticed that Safari doesn't handle the zoom function as well as Chrome. At least for me, it zooms the whole page rather than just the structure. Is there a different way to zoom it in Safari, and can we copy the Chrome style? (Also, I have to say again that the behavior on mobile is awesome!)

KianBadie commented 3 years ago

@coiko That is odd that you are getting that error. This might be a reach, but I just wanted to make sure: have you recently built and served the site from your own computer? If not, then there might be a trickier issue going on behind the scenes that I will get in contact with Tom about.

Oh that is also interesting, I will try to replicate that and investigate what is going on and try to make things consistent. That is awesome to hear that mobile is doing well!

coiko commented 3 years ago

Thanks @KianBadie! I haven't built/served the site for quite a while (certainly not since you added the protein viewer), so it might be something behind the scenes? Thanks for looking into it before we move things onto the main site!

KianBadie commented 3 years ago

@coiko I see, I will do some digging and try to find out what is going on. On the main site, I don't think this will be an issue since cross origin errors don't happen since it is on the same server. But I still want to get to the bottom of this since it is a real problem. I will let you know of any findings.

Also, I tried to replicate the scrolling/zooming behavior and was not able to do so. Was this on mobile or on desktop? And when zooming, is it by just using the mouse wheel or are any other buttons being used?

coiko commented 3 years ago

@KianBadie Ah, sorry, I left out some critical information! It's a Mac laptop (or trackpad, I guess)-specific problem. A mouse with a scroll wheel works great. But on the trackpad, if you do the pinch-expand gesture, on Chrome it has the same behavior as the mouse wheel (zooming just the structure), but on Safari it zooms the whole page. Don't worry about it if it's not an easy fix - we can certainly leave it to the future, but I just wanted to bring it up while it occurred to me. Thanks!

(Also feel free to ignore the cross origin error on the protein structures since it won't happen on the main site!)

KianBadie commented 3 years ago

@coiko No worries! Ah I see, that's interesting that the two treat the same device differently. And sounds good! In that case I will lower it on my priority list for the next update and continue to work on other things, but will come back to it in the future.

Sounds good as well with the cross origin issue!

KianBadie commented 3 years ago

@coiko I think I figured out why the PDB numbering was weird on some proteins. As you know, the protein number is determined by the qualified name (for example "J.TYR83.CA") and that the first character is what determines the number. However, I found that a protein with a weird protein number had the qualified name "3.VAL93.CA". The way I wrote things, the label assumed only alphabetical characters would be there but did not account for numbers.

Are numbers an expected thing in the qualified name for the protein? Or is this a PV thing? If it is a PV thing, I can try to investigate how they arrived at giving a number for the protein. If it is expected, where do numbers fall on the protein number ordering? Is "1" before "A" and what would numbers translate to?

coiko commented 3 years ago

@KianBadie Ah, that makes sense - thanks! Yes, unfortunately numbers are an expected identifier for proteins (along with letters). When structures get really big (e.g. like 6kgx), they use capital letters (A-Z), then lowercase letters (a-z), then numbers (1-9) to identify up to 61 proteins. (If even more identifiers are needed, they use a two-character system, doubling them up in various configurations, like A1-9, B1-9...a1-9...Aa-z... but we shouldn't have to deal with that.) Sorry I forgot to tell you about this earlier (I assumed PV was handling the numbering)!

When you have the time and inclination to fix it (feel free to wait - this is very low priority!), the translation should be: A-Z = 1-26, a-z = 27-52, 1-9 = 53-61. (As a further wrinkle, some structures skip some intervening letters/numbers so you'll have e.g. A, B, C and F, so if you ever feel really ambitious, you could also clean those up to consecutive numbers, but that's really not necessary.)

KianBadie commented 3 years ago

@coiko Ah I see, and no worries! If it is that low priority, I will let you know if I get around to it. I will probably just create a look up table for that. I think accounting for the skipping would for sure have to wait for the next update, as that will take a little bit more time/logic.

Also, I pushed some changes that improved the protein highlighting. At least on my computer, the selected protein seems to follow the tip of the pointer. Unfortunately, it did not look as accurate when I tested in Safari, but it did look like it improved from the previous version. Please let me know if things seem the same to you. The problem ended up being on my end. I'm using the built in PV functionality that selects a residue when given a certain position. The clicked position I was giving was not accounting for the borders of the viewer.

coiko commented 3 years ago

@KianBadie Thank you so much! It looks perfect to me now in Chrome, and much, much better in Safari.

(And yes, I think it's great to wait on the numbering issue.)

KianBadie commented 3 years ago

@coiko I uploaded some changes to use a look up table that covers A-Z, a-z, and 1-9. It looks like it is being used as intended, but something still look off. For example, in the 4.3 PDB, some of the proteins are numbered as 56. I'm not sure if it is correct, if there is something with the file, or PV because 56 seems like a high jump when the other proteins sit around the 10s area. If you notice any thing that looks off, please let me know!

KianBadie commented 3 years ago

@coiko In addition, I noticed that PDB 609J had "undefined" for the protein name because they did not exist in our protein look up table. Some of the names that were not in the table were "N1", "C3", and "N3". One small/easy thing I could do is to just not display a protein name for proteins that aren't in our look up table. Another option is to add more entries into the table. What are your thoughts?

KianBadie commented 3 years ago

@coiko I was testing the protein viewer on iOS and had some new thoughts on the full-screen functionality that it has. For a refresher, iOS does not have true full-screen for non-video elements, so we made our own implementation. Because of this however, the nav bars get in the way of the top/bottom of the full-screen elements sometimes. In the past, I added some functionality to resize the element when the nav bars come in/out for the comparison slider, but as we discussed that causes bugs with zooming so we removed it for future elements. The protein viewer is included in this.

However, I did notice that on portrait, it was a little bit jarring for the nav bars to pop up when I was trying to click the "X" in the bottom left corner, and then covering that element (so I had to switch to landscape to actually exit the viewer). One potential "fix" that I thought of was to move the "X" to the top right corner on mobile only. Most of the full-screen elements on mobile have the "X" in the top right, while having it in the bottom right for desktop (with an exception for the image elements in the modals). So making this change would somewhat add consistency. What are your thoughts?

Ideally in the future, I would make things in a way where this bug wouldn't happen in the first place. However, I think now is a little too late to introduce functionality like that without fully testing the side affects.

coiko commented 3 years ago

@KianBadie sorry for the delayed response, but thanks so much for all this!

As for the numbering, that PDB in 4.3 (6kgx) is a beast and uses the combo two-digit/letter system (basically every combination of upper/lower/numbers) to label a few hundred different proteins. So when I said "but we shouldn't have to deal with that" I was very wrong and had totally forgotten about how big this thing is. I'm sorry! That said, I definitely wouldn't try to fix it now. I think we can look into it down the road and for now, the main thing is that the residue selector works.

As for the undefined residues in 6O9J, good catch! I had also forgotten about them. It's a weird hybrid structure that's part protein and part RNA (a different macromolecule). The residue selector only makes sense for the protein part of it, so yes, I think the fix you proposed of just not identifying anything not in the lookup table is the perfect way to handle it. Thanks!

Finally, for the full-screen functionality, I really like the idea of moving the "X" to the top right corner on mobile. (As an added bonus, I think the top-right corner is more intuitive from other sites.) So yes, please go ahead with that.

KianBadie commented 3 years ago

@coiko No worries and no problem!

No worries and it sounds good to me to save the numbering fix for later.

Sounds good. When you say "not identifying anything not in the lookup table", does that mean no label at all? At first I was thinking of displaying "Protein 2 | Residue #5 | undefined" as "Protein 2 | Residue #5", but your explanation makes it seem like there shouldn't be anything displayed.

I will go ahead and move it to the top right!

coiko commented 3 years ago

Thanks @KianBadie! And yes, I think it would be best not to display anything since it's not actually a protein (unless it's more of a pain to set it up that way, in which case we can definitely go with "undefined").

KianBadie commented 3 years ago

@coiko Now that I think about it, do you think it would be fitting if something was displayed? I ask because I was about to implement showing nothing at all, but then I thought of the scenario that somebody clicks something, the structure highlights, but no label shows. I was thinking it might come off as a bug. Then I thought that if those structures were not highlightable, then it might also give off a wrong impression. What are your thoughts on displaying some sort of phrase that denotes that the highlighted structure is not a protein? Or do you think it would not fit with the purposes of the viewer?

coiko commented 3 years ago

@KianBadie Yes, that's a great idea! Maybe it could be the same style as the "water" label and just say "non-protein". What do you think?

KianBadie commented 3 years ago

@coiko I think that sounds great! "non-protein" it is.

KianBadie commented 3 years ago

@coiko I just pushed some changes to display that custom "non-protein" message if the highlighted atom does not exists in our protein look up table. Please let me know if you notice any future issues with that!

coiko commented 3 years ago

@KianBadie Wonderful! Thank you so much!

Also, I just talked with Grant and it looks like the viewer doesn't show up in Firefox (it just shows a funny semi-transparent window with the display options at the top). So maybe we can add troubleshooting that to the to-do-sometime-but-not-necessarily-before-this-update list?

KianBadie commented 3 years ago

@coiko No problem! I also just pushed some changes to remove ball and sticks/volume filling rendering on mobile.

And oh I see that is unfortunate if PV and firefox do not work with each other. Yes, that sounds good to me. I'll do a quick check right now and see if something else is going on.

coiko commented 3 years ago

Thanks @KianBadie! It looks great!

KianBadie commented 3 years ago

@coiko I ended giving some more time to the bug Grant was experiencing and just pushed some changes now! It looks like the problem was something silly. I was using a property that only Chrome and Mac understood to calculate the size of the viewer. Because Firefox did not understand it, it rendered the viewer with no dimensions.

coiko commented 3 years ago

@KianBadie Oh wonderful! Thank you so much!