Skip to content
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

add dash-ngl #496

Merged
merged 63 commits into from
Oct 15, 2020
Merged

add dash-ngl #496

merged 63 commits into from
Oct 15, 2020

Conversation

IvoLeist
Copy link
Contributor

@IvoLeist IvoLeist commented Mar 5, 2020

About

  • I am closing an issue
  • This is a new component
  • I am adding a feature to an existing component, or improving an existing feature

Description of changes

as discussed with @rpkyle he wanted me to do a PR to add my
dash component https://github.com/IvoLeist/dash_ngl
to dash-bio to replace moleculeViewer3d

It is inspired by the shortcomings of https://dash.plot.ly/dash-bio/molecule3dviewer which does not support .cif files and has issues in showing a merged pdb structure: any protein + DNA #467

Features

  • Protein selection by dropdown
  • Support for .pdb (.gz) and .cif(.gz) files
  • Show multiple structures side by side
  • Filter the structure to show only one chain (e.g. 6CHG.A)
  • Filter the structure to show only a residues range of one chain (e.g. 6CHG.A:1-450)
  • Changeable viewer background (black / white)
  • Changeable camera settings ( perspective / orthographic)
  • Changeable chain colors
  • Changeable molecular representations (cartoon, sticks etc.)
  • Highlight of chosen Atoms (represented as balls)
  • Highlight C alpha of chosen residues (represented as balls)
  • Download of the structure as png

Limitations

So far only 2-3 structures at the same time are supported

  • A structure can only be filtered to one chain
  • Placement and zooming is far from perfect
    zipped files are not (yet) supported

Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an awesome component! Thank you for taking the time and effort to help us integrate it into dash-bio. I just have a few questions/concerns to be addressed - once those are taken care of, I can do another sweep through the code.

package.json Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
style={"backgroundColor": "#3aaab2", "height": "7vh"},
),
viewer,
# if you use dcc.Loading viewer the app goes into a mount loop!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please elaborate on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting this bug magically vanished at least on my laptop (Brave)*
I am going to add dcc.Loading in another commit.

*It might still not work on my office machine (Chromium)
going to try that on Monday

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shammamah unfortunately the bug did not vanish completely the mount loop which leads to crashing the app is gone. However, I realized that my feature: enabling reload from browser when molecule was selected before is not working since the component gets remounted after every new selection.

If you want to test on your own I updated my dash_ngl component
just uncomment dcc.Loading and you will see the component did mount
console.logs appearing after each new selection :-/

tests/dashbio_demos/dash-ngl/app.py Outdated Show resolved Hide resolved
@shammamah-zz
Copy link
Contributor

Please also make sure to run pylint on the app that you've created - that will get rid of quite a few of those Python CI test failures! :)

@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

Just added another commit adressing @shammamah 's concern.
Thank you for the quick first review and the positive feedback :)

Do not wonder regarding the ci failure it is supposed to fail since
I have not found the time yet to update test_DashNgl.py

src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
loadStructure(stage, filename, chain, color, xOffset) {
console.log('load from browser');
// console.log(filename)
const stageObj = stage.getComponentsByName(filename).list[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please explain what filename is here, exactly/what the getComponentsByName function does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filename is literally the filename how it is named in the data folder (e.g. 6CHG.pdb). It is taken from the information coming from the backend to the frontend. These are collected in componentDidUpdate:
const {data, stageParameters} = this.props;

And the filename is attached in the loadData method to the stageObj see code snippet below

loadFile(stringBlob, {ext: data.ext, defaultRepresentation: false}).then(stageObj => {
                stageObj.name = data.filename;

This naming of the stageObj. is then the basis for the method loadStructure with its getComponentsByName function

Maybe this snippet makes the whole process more clear:
https://github.com/arose/ngl/blob/e8713514cfa7100a8313b3ff34a3c94b4125ced4/examples/scripts/component/getByName.js

The idea behind all this as soon as someone picks a molecule it is loaded and saved in the stageObj. which isn't cleared as long the user does not reload the page. So if the user selects some different molecules but from time to time comes back to its very first selection. it's structure is still loaded in the application so there is no need for another backend call.

This is really important for our specific use case since we want the webserver calls as low as possible and so far it did not lead to too high ram usage of the browser. However I am not sure if that approach would be suitable for mobile users as well.

Up to now we did not any stress tests yet. Meaning how much structures I can load into the stage till this system breaks would be interesting to get to know the limitations. 🗡️

src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

@shammamah I just finished my first python test :)
Is it possible to test it locally or should I just push it ?

mostly comments improved only one JS code change

Co-Authored-By: Shammamah Hossain  <shammamah.hossain@gmail.com>
@shammamah-zz
Copy link
Contributor

@IvoLeist That's great to hear! You should be able to run it locally with pytest -k tests/integration/yourtest.py. Make sure that you check out the testing portion of the CONTRIBUTING.md if you get stuck: https://github.com/plotly/dash-bio/blob/master/CONTRIBUTING.md#step-3-run-tests-locally

@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

@shammamah sorry to bother you again but unfortunately I am still stuck with local testing...

pip install dash[testing]
zsh: no matches found: dash[testing]
``

@shammamah-zz
Copy link
Contributor

@IvoLeist No worries! pip might be installing packages to a Python 2 environment; could you try again with pip3 install dash[testing]? (And make sure you're on the correct version of Python by running python --version)

@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

@shammamah
❯ python --version
Python 3.7.6
❯ pip3 install dash[testing]
zsh: no matches found: dash[testing]

@shammamah-zz
Copy link
Contributor

Ah, I see what's going on! Try escaping the square brackets: https://stackoverflow.com/questions/30539798/zsh-no-matches-found-requestssecurity

So run

pip install dash\[testing\]

@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

@shammamah thanks for the quick google.
I had no idea about this issue regarding zsh (recently switched to it)

almost there... :)

Now it fails because it does not find dash_bio ?

―――――――――――――――――――――――――――――――――――――――――――――― tests/integration/test_volcano_plot.py ――――――――――――――――――――――――――――――――――――――――――――――
ImportError while importing test module '/home/ivo/projects/bioinfo/repos/dash-bio/tests/integration/test_volcano_plot.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/integration/test_volcano_plot.py:6: in
import dash_bio
E ModuleNotFoundError: No module named 'dash_bio'

pip install dash-bio
Requirement already satisfied: dash-bio in /home/ivo/miniconda3/lib/python3.7/site-packages (0.4.7)

@shammamah-zz
Copy link
Contributor

This might be due to the pip/pip3 thing I mentioned earlier. Could you try pip3 install dash-bio?

@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

@shammamah ah with this pip/pip3 it is so confusing :D
Why do you not support conda (?)

The local test run in 54 errors most of them related to the selenium driver
I guess I have to figure out how to install it.

@IvoLeist
Copy link
Contributor Author

IvoLeist commented Mar 6, 2020

@shammamah update chromedriver was easy to get
*I love Aur 💃

But there is unfortunately a new error...
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

―――――――――――――――――――――――――――――――――――――――――――――――――― tests/unit/test_clustergram.py ――――――――――――――――――――――――――――――――――――――――――――――――――
import file mismatch:
imported module 'test_clustergram' has this file attribute:
/home/ivo/projects/bioinfo/repos/dash-bio/tests/integration/test_clustergram.py
which is not the same as the test file we want to collect:
/home/ivo/projects/bioinfo/repos/dash-bio/tests/unit/test_clustergram.py
HINT: remove pycache / .pyc files and/or use a unique basename for your test file modules

Edit:
just running pytest -k tests/integration/test_dashNgl.py did not help

Edit2:
Testing the existing one does also not work
e.g. pytest -k tests/integration/test_molecule3d.py
=> Same error

@shammamah-zz
Copy link
Contributor

@IvoLeist Oops, I misled you... you should just be running

pytest tests/integration/test_dashNgl.py

The -k option means that you're specifying a keyword, so it makes sense that pytest doesn't know whether to run the unit test or the integration test for clustergram: https://docs.pytest.org/en/latest/usage.html

Sorry about that - could you just try running pytest without the -k option and let me know if that works?

@shammamah-zz
Copy link
Contributor

Re: this:

Why do you not support conda (?)

The best answer I can give you is that we just haven't had the time/resources to seriously look into it. We do know that a lot of people use conda, though!

Copy link
Contributor

@shammamah-zz shammamah-zz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few things -- the viewport issue should be resolved now. I used the setSize function from here: http://nglviewer.org/ngl/api/class/src/stage/stage.js~Stage.html#instance-method-setSize

src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
src/lib/components/DashNgl.react.js Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
R/dashbioDashNgl.R Outdated Show resolved Hide resolved
viewportStyle: PropTypes.exact({
width: PropTypes.string,
height: PropTypes.string,
}),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we break this out into two props, width and height, each of which is just a number? The current implementation expects a CSS string, which is indeed more flexible but it's harder to use for non-web-devs. If you think this flexibility is important we could support both, like widthStr = isNumeric(width) ? (width + 'px') : width (using my favorite fast-isnumeric 😉 so 500 and '500' count as numbers, ie pixels, but any other string is a CSS size string) or we can start with just numbers and include strings later if there's interest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 78fd7e9.

Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
NAMESPACE Outdated Show resolved Hide resolved
@alexcjohnson
Copy link
Collaborator

Apparently the component works without it, but we should add setProps as an explicit prop.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one final comment about the version number, then let's go! 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants