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

Feat/nglview tooltip #600

Merged
merged 5 commits into from
Oct 6, 2019
Merged

Feat/nglview tooltip #600

merged 5 commits into from
Oct 6, 2019

Conversation

umesh-timalsina
Copy link
Member

@umesh-timalsina umesh-timalsina commented Oct 3, 2019

PR Summary:

To address #443, this PR makes the following contributions:

  1. Adds function overwrite_nglview_default in the file jsutils for js tooltip behavior modifications
  2. When hovering on atom, the tooltip now has atom.index, from ngl as the feature.

PR Checklist


  • Includes appropriate unit test(s)
  • Appropriate docstring(s) are added/updated
  • Code is (approximately) PEP8 compliant
  • Issue(s) raised/addressed?

Some Examples:
image

new_example

@codecov
Copy link

codecov bot commented Oct 3, 2019

Codecov Report

Merging #600 into master will increase coverage by <.01%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #600      +/-   ##
==========================================
+ Coverage   90.25%   90.26%   +<.01%     
==========================================
  Files          48       49       +1     
  Lines        3696     3707      +11     
==========================================
+ Hits         3336     3346      +10     
- Misses        360      361       +1
Impacted Files Coverage Δ
mbuild/utils/jsutils.py 100% <100%> (ø)
mbuild/compound.py 91.24% <90%> (-0.08%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e8d0b0...01084f5. Read the comment docs.

@umesh-timalsina umesh-timalsina marked this pull request as ready for review October 4, 2019 14:43
Copy link
Contributor

@justinGilmer justinGilmer 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 really cool! I had a comment or two, but no changes.

Did you run into any issues installing and using nglview to visualize any compounds?

The reason we made default py3dmol was due to a lot of us having issues where the visualizations never appeared, which appeared to be a mix of issues ranging from the Jupyter widgets themselves, to having to delete certain folders that the widgets were using.

Can you try making a new anaconda env, using the default installed nglview, and see if you run into any visualization issues? With your knowledge of JS, you might be able to discover something we missed.

Ill try it on my machine too, and see what happens

LGTM!

"""
this.stage.mouseControls.add('hoverPick', (stage, pickingProxy) => {
let tooltip = this.stage.tooltip;
if(pickingProxy && pickingProxy.atom && !pickingProxy.bond){
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to satisfy my curiosity, if we support more features from nglview, would this args section essentially be extended?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. It depends on the type of support nglview has in the future. If they have additional python methods in the future, we wouldn't have to rely on this js call. There are a lot of features in ngl which cannot be implemented directly from nglview without firing js.

@umesh-timalsina
Copy link
Member Author

umesh-timalsina commented Oct 4, 2019

@justinGilmer As of right now @ahy3nz ran this branch in jupyter and ran into problems with widget initialization(see #600 (comment)) . Its running fine in linux but macos is running into problems.
As it relates to newvenv, I am not running into any problems in my Ubuntu 18.04.

conda create -n nglview-test
conda activate nglview-test
conda install -c conda-forge nglview
conda install nglview -c conda-forge
jupyter nbextension enable --py --sys-prefix widgetsnbextension
jupyter nbextension enable --py --sys-prefix nglview
jupyter notebook

Python version 3.7
Seems to produce visualizations without issue in both chrome and firefox for ubutnu.
image

@hainm
Copy link

hainm commented Oct 4, 2019

As of right now @ahy3nz ran this branch in jupyter and ran into problems with widget initialization. Its running fine in linux but macos is running into problems.

It's uncommon to see user getting issue with nglview nowadays. Since I develop nglview on my Macos, it should work too. If trying nglview in a new nglview-test (like your commands above) and the widget is not shown, I suspect that user still have very old distribution of nglview 0.6.x that was unfortunately installed in $HOME/.jupyter folder. If so, deleting the folder would resolve the case and the issue won't happen in the future.
https://github.com/arose/nglview/blob/master/docs/FAQ.md#widget-not-shown

@ahy3nz
Copy link
Contributor

ahy3nz commented Oct 4, 2019

I think on previous attempts with nglview, I've tried something similar with the removals

(base) ayang41@DeltaDurian:~/.jupyter$ ls
custom/   migrated  nbconfig/
(py36) ayang41@DeltaDurian:~/.jupyter$ cd ~/.local/
(py36) ayang41@DeltaDurian:~/.local$ ls
bin/ lib/

@hainm
Copy link

hainm commented Oct 4, 2019

hi @ahy3nz: Do you mind trying it now with a fresh conda env and the latest nglview (commands from @umesh-timalsina's post above). I just want to resolve this issue once for all.

If the widget is not shown, can you please post the Javascript console (Chrome: Right click -> Inspect -> Console).

Thanks.

@ahy3nz
Copy link
Contributor

ahy3nz commented Oct 4, 2019

Within a fresh 37 environment, having only installed nglview, with the 2 nbextensions enabled:

image

@ahy3nz
Copy link
Contributor

ahy3nz commented Oct 4, 2019

I got it to work, I kind of went on a rampant witch hunt to find nglview-js-widgets in a variety of folders in my (messy) mac, and I think this removal was the one that got things to work:

(nglview-test) ayang41@DeltaDurian:~/Library/Jupyter/nbextensions$ rm -rf nglview-js-widgets/

image

@hainm
Copy link

hainm commented Oct 4, 2019

~/Library/Jupyter/nbextensions$ rm -rf nglview-js-widgets/

oh, @ahy3nz you are right. Jupyter use that $HOME/Library/Jupyter on macos rather the $HOME/.local. I've updated the nglview's FAQ and thanks for hunting this down. cheers.

@hainm
Copy link

hainm commented Oct 4, 2019

~/Library/Jupyter/nbextensions$ rm -rf nglview-js-widgets/

Once this is resolved, the issue won't happen with future versions since nglview-js-widgets is now installed to $PREFIX/share/jupyter/nbextensions/nglview-js-widgets which is always overwritten by installing newer version.

Copy link
Contributor

@ahy3nz ahy3nz left a comment

Choose a reason for hiding this comment

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

image
Looks good to me with nglview and showing atom indices in the hover tooltip, I don't know if you also wanted to put atom indices in the upper-left text when you click on an atom. I'll also observe that residue/compound naming won't work in the visualizer because the mb.compound is implicitly converted to a mdtraj.Trajectory or pmd.Structure with no kwargs for residues to specify residue names, and the resultant mol2 file has no residue names besides the default RES

@umesh-timalsina
Copy link
Member Author

umesh-timalsina commented Oct 4, 2019

I don't know if you also wanted to put atom indices in the upper-left text when you click on an atom.

Seems doable to me. (01084f5)

I'll also observe that residue/compound naming won't work in the visualizer because the mb.compound is implicitly converted to a mdtraj.Trajectory or pmd.Structure with no kwargs for residues to specify residue names, and the resultant mol2 file has no residue names besides the default RES.

So, according to ngl/fileformats, basically all of our writers are supported by ngl. Can we map out a clear way to make this happen from one of our writers?

@mikemhenry
Copy link
Member

I had some issues with juypter lab but following the steps here: nglviewer/nglview#845 I was able to get it to work!

@uppittu11
Copy link
Member

I also got nglview + the tooltip working thanks to Alex's fix. And it worked fine on all of our example notebooks except the the coarse grain notebook (mbuild/examples/coarse_graining/cg_hexane.ipynb). Is this expected? (Py3dmol worked fine for this example by the way)

The Lennard Jones example worked so I'm guessing this isn't an issue with having custom element types.

image

@mattwthompson
Copy link
Member

Hey, this worked for me too! Thanks @ahy3nz @hainm!

image

I did something like this:

$ conda create -n nglview-test python=3.6 nglview mbuild -yq
$ conda activate  nglview-test
$ rm -rf ~/Library/Jupyter/nbextensions/nglview-js-widgets/

@mattwthompson mattwthompson merged commit 87eddd5 into mosdef-hub:master Oct 6, 2019
umesh-timalsina referenced this pull request in GOMC-WSU/MoSDeF-GOMC Mar 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants