T017 - Advanced NGLview usage #92
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Hi @jaimergp, I had a quick look at the table of contents as requested - very well done! First things first:
Things that come to mind for 1.3.2:
W.r.t. 1.3.3 I do not know really what will live behind the titles, so here is what I would like to learn maybe?
Looking forward to this notebook - it will be helpful for everyone who starts out using |
|
Only theory missing! 🥳 |
|
Theory done. I need to update the table of contents and polish for typos and so on, but this will be ready for review later today! |
|
@dominiquesydow, this is ready for review! |
|
Excellent! On my agenda for tomorrow :) |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:47Z If we wanted to be in-line with the other notebooks, this would be
|
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:48Z Style for references in T001-T010 notebooks - example:
|
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:49Z This is great! Thanks for this clear explanation!
Some typos:
insallation > installation offers provides > provides Some of the most advanced features > Some of the more (?) advanced features |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:49Z Ah! Did not know that distances are representations, too! The representations-coordinates link to remember this is brilliant!
A component can be a molecular structure or a geometric shape. Components can be rendered with one or more representations. Maybe put this first representations mention in italic (instead of later).
Put first mention of shape in italic, too?
To make sure you got the gist, you can also check the examples in the demo application and get more familiar with the NGL terminology. Where would I find more details on components/representations/shapes following this link? Or were you thinking people can simply start using the viewer and play around (asking because I was expecting to see something like a schema).
jaimergp commented on 2021-04-13T13:45:24Z Added one more sentence to clarify:
To make sure you got the gist, you can also check the examples in the demo application (dropdown in the top menu) and get more familiar with the NGL terminology. Each example will load a state on the 3D canvas, with components being on the top level of the hierarchy, and their representations under each one. dominiquesydow commented on 2021-04-13T15:35:56Z Perfect! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:50Z Simply beautiful :) |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:51Z Cool to know how this can be done with |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:51Z Thanks for that explanation! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:52Z Do you know why it is not possible to export the canvas from the JavaScript layer to the cell output directly?
Minor: Add dot after 2nd and 3rd bullet points. jaimergp commented on 2021-04-26T12:28:17Z No clue why. It might be a JS/Python IO buffer thing. Not sure why but if at this point it hasn't been "fixed", it might be unfixable with the current design.
Thanks for the dots! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:53Z If you combine all those implies to me that we have talked about jaimergp commented on 2021-04-13T13:47:51Z Good catch! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:54Z That doesn't mean the structures were not loaded Would it be correct to say That doesn't mean the structures were not loaded (the components are loaded but no representation is set, yet). If so, this could be a nice circle back to your introduction. jaimergp commented on 2021-04-13T13:49:37Z Excellent! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:54Z I know it is a bit annoying, but in this first case where you show this, I would split this cell (only this once) into
cell 1
cell 2
Would be nice for the Jupyter Lab usage of this notebook.
... or you comment here that the view will be updated above (you do this later in this notebook). |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:55Z Interesting. So I am seeing these stars when I am drawing "lines" using only one atom? jaimergp commented on 2021-04-13T13:53:28Z Apparently that's how the shader works 🤷 |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:38:55Z with a larger radius larger than what? Would it be correct to say sphere (or does it look like sphere but technically is not a sphere)? as a sphere
also the Iridium ion sounds like we have talked about the structure before looking at it. So maybe add a short description about the type of protein we look at before you start displaying it?
Thinking about this - why did you choose that structure? (Unhappy about kinases? :) They would have protein+ligand+Mg as well) jaimergp commented on 2021-04-13T14:07:03Z It's a PDB with sentimental value in my PhD :D I know how it should look like and how it to get a good representation of its features. I don't think we need to go into many details about it, but I added a small sentence at the beginnign of the section. dominiquesydow commented on 2021-04-13T15:50:40Z Happy with the changes! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:39:07Z
|
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:39:08Z a f-string > an f-string |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:39:09Z Ah! Can you explicitly add that this code (
you can inspect the to something like this you can type there
Thanks for all the tips and pitfalls! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:39:09Z This is a nice and clean overview, thanks! |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-02-17T15:39:10Z sidechains > side chains
People will need to know how to access KLIFS programmatically for this, right? If so, could you link to the KLIFS talktorial T013? |
|
@jaimergp, this notebook is amazing on so many levels! My brain is fried from all the new things I learnt. Great job! And thank you ❤️ |
|
Added one more sentence to clarify:
To make sure you got the gist, you can also check the examples in the demo application (dropdown in the top menu) and get more familiar with the NGL terminology. Each example will load a state on the 3D canvas, with components being on the top level of the hierarchy, and their representations under each one. View entire conversation on ReviewNB |
|
Good catch! View entire conversation on ReviewNB |
|
Excellent! View entire conversation on ReviewNB |
|
Apparently that's how the shader works 🤷 View entire conversation on ReviewNB |
|
It's a PDB with sentimental value in my PhD :D I know how it should look like and how it to get a good representation of its features. I don't think we need to go into many details about it, but I added a small sentence at the beginnign of the section. View entire conversation on ReviewNB |
|
Hm, apparently not all classes are fully documented... I clarified that and your other comments! View entire conversation on ReviewNB |
|
I added a sentence clarifying the intent, but we can change to other enzyme if you have a clearer example at hand! View entire conversation on ReviewNB |
|
No clue... Let me know if you get it work and we can add a sentence! View entire conversation on ReviewNB |
|
Without fixed, interact will consider every option a thing that needs to be modified by widgets. View entire conversation on ReviewNB |
Co-authored-by: dominiquesydow <dominiquesydow@users.noreply.github.com>
|
@dominiquesydow, thanks for the thorough review!! Took me ages but I have started reviewing your comments :) I have resolved most of the feedback, but there are some outstanding issues I need some help and/or clarification! Thanks again! |
|
Perfect! View entire conversation on ReviewNB |
|
Great, motivation is clear and unused PDB is gone - no need to change enzymes here. View entire conversation on ReviewNB |
|
Ok, question was pure curiosity. If I find that out, I'll let you know :) View entire conversation on ReviewNB |
|
Ok, I admit the word "non-dynamic" says it all. Was lacking mental flexibility here, thanks for clarifying! View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB dominiquesydow commented on 2021-04-13T15:45:07Z Iridiu > Iridium |
|
Excellent! Unsure what the open issues are - I did comment on hopefully everything you wrote. Basically everything a ready-to-go :) This talktorial helped me tremendously in another project with NGLview and a lot of |
|
Happy with the changes! View entire conversation on ReviewNB |
|
No clue why. It might be a JS/Python IO buffer thing. Not sure why but if at this point it hasn't been "fixed", it might be unfixable with the current design.
Thanks for the dots! View entire conversation on ReviewNB |
|
You can use hex colors like View entire conversation on ReviewNB |
Co-authored-by: Dominique Sydow <dominiquesydow@users.noreply.github.com>
|
Ok, we are done! Thank you so much for the review @dominiquesydow! |
Details
Content review
here.DataFrames)Code review
a_variable_namevsaVariableName)black -l 99)for i in range(len(list))(see slides)# TODO: CIimport ...lines are at the top (practice part) cell, ordered by standard library / 3rd party packages / our own (teachopencadd.*)