-
-
Notifications
You must be signed in to change notification settings - Fork 193
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
add dash-ngl #496
Conversation
There was a problem hiding this 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.
tests/dashbio_demos/dash-ngl/app.py
Outdated
style={"backgroundColor": "#3aaab2", "height": "7vh"}, | ||
), | ||
viewer, | ||
# if you use dcc.Loading viewer the app goes into a mount loop! |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :-/
Please also make sure to run |
Just added another commit adressing @shammamah 's concern. Do not wonder regarding the ci failure it is supposed to fail since |
src/lib/components/DashNgl.react.js
Outdated
loadStructure(stage, filename, chain, color, xOffset) { | ||
console.log('load from browser'); | ||
// console.log(filename) | ||
const stageObj = stage.getComponentsByName(filename).list[0]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. 🗡️
@shammamah I just finished my first python test :) |
mostly comments improved only one JS code change Co-Authored-By: Shammamah Hossain <shammamah.hossain@gmail.com>
@IvoLeist That's great to hear! You should be able to run it locally with |
@shammamah sorry to bother you again but unfortunately I am still stuck with local testing...
|
@IvoLeist No worries! |
@shammamah |
Ah, I see what's going on! Try escaping the square brackets: https://stackoverflow.com/questions/30539798/zsh-no-matches-found-requestssecurity So run
|
@shammamah thanks for the quick google. almost there... :) Now it fails because it does not find dash_bio ?
|
This might be due to the |
@shammamah ah with this pip/pip3 it is so confusing :D The local test run in 54 errors most of them related to the selenium driver |
@shammamah update chromedriver was easy to get But there is unfortunately a new error...
Edit: Edit2: |
@IvoLeist Oops, I misled you... you should just be running
The Sorry about that - could you just try running |
Re: this:
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! |
There was a problem hiding this 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
removed unnecessary square brackets from Output Co-Authored-By: Shammamah Hossain <shammamah.hossain@gmail.com>
viewportStyle: PropTypes.exact({ | ||
width: PropTypes.string, | ||
height: PropTypes.string, | ||
}), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
Apparently the component works without it, but we should add |
There was a problem hiding this 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! 💃
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
…nto add-dash-ngl
About
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
Limitations
So far only 2-3 structures at the same time are supportedzipped files are not (yet) supported