Draw bonds computed by ASE #535

yakutovicha · 2023-11-06T16:35:12Z

fixes #48
supercedes #377

In nglview if a system is too big bonds will not be computed.
I bypass the "ball+stick" representation defining it as "spacefill" plus shapes that are computed via ase connectivity matrix.

I would not draw also the atom spheres through shapes otherwise it would be a problem to pick atoms with mouse selection

Despite I tested it also with ~1000 atoms it is meant to work on smaller system sizes.

The image below is outdated, just the "ball+stick" representation is needed to produce the image

To Do:

Consider using Enum for the dropdown widgets.

codecov · 2023-11-07T16:15:41Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (f403330) 85.88% compared to head (0cd8708) 87.12%.

Files	Patch %	Lines
aiidalab_widgets_base/viewers.py	98.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #535      +/-   ##
==========================================
+ Coverage   85.88%   87.12%   +1.23%     
==========================================
  Files          27       27              
  Lines        4618     4643      +25     
==========================================
+ Hits         3966     4045      +79     
+ Misses        652      598      -54

Flag	Coverage Δ
python-3.10	`87.12% <98.57%> (+1.23%)`	⬆️
python-3.8	`87.15% <98.57%> (+1.23%)`	⬆️
python-3.9	`87.15% <98.57%> (+1.23%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…here they were.

…ab/aiidalab-widgets-base into feature/draw-atomic-bonds

yakutovicha · 2023-11-09T08:57:55Z

[ ] Consider using Enum for the dropdown widgets.

This item is not yet implemented, but I think we can discuss it first during today's meeting and then decide what to do.

The rest is ready, please review.

superstar54

Hi @yakutovicha Thanks for the work. It's important for the large structure. I made a few change requests.

Also two comments:

better to add a test for the _compute_bonds function.
There is a variable called uuid, which is the Unique identifier for the representation.. At first glance, I mixed it with the AiiDA node uuid. I suggest using another name, e.g. style_id.

aiidalab_widgets_base/viewers.py

superstar54 · 2023-11-10T13:34:40Z

aiidalab_widgets_base/viewers.py

+
+        radius = radius / 5
+
+        cutoff = ase.neighborlist.natural_cutoffs(structure, mult=1.09)


Also a comment on why you choose 1.09.

Same as above, while testing on a number of systems in some cases I had bonds where I did not 'want' them in in other cases I did not have bonds and I wanted them. This 1.09 was a good compromise. Feel free to change to a value that produces even better results.

done in 6e547fc

aiidalab_widgets_base/viewers.py

superstar54 · 2023-11-10T14:02:27Z

aiidalab_widgets_base/viewers.py

+                half_bond_point = v1 + bond_length * Radius[atom1.symbol] / (
+                    Radius[atom1.symbol] + Radius[atom2.symbol]
+                )


The math here is wrong. You need both the vector and the length to get the middle point.

The naming here is confusing, I guess. This is to place the point where the bond changes colour. We find this point using the proportion of the atomic radius of the atoms.

I renamed the variable to intermediate_point in d4f0a06

We find this point using the proportion of the atomic radius of the atoms.

No. The correct math is:

# get the bond_vector from the `neighbor_list` too. normal = bond_vector/bond_length r1 = Radius[atom1.symbol] r2 = Radius[atom2.symbol] intermediate_point = v1 + r1*normal + (bond_length - r1 - r2)/2*normal # you can use the short term: intermediate_point = v1 + (bond_length + r1 - r2)/2*normal

I don't understand what is wrong with our approach.

pos1 - location of the first atom
pos2 - location of the second atom
bond_vector is a vector connecting the first atom and the second atom.
r1 - is the vdW radius of the first atom
r2 - is the vdW radius of the first atom

The intermediate point is a percentage of the vector r1 relative its contribution to the sum of two vdW radii.

bond_vector = pos2 - pos1

atom1_contribution = r1 / (r1 + r2)
atom2_contribution = r2 / (r1 + r2)

intermediate_point = pos1 + bond_vector * atom1_contribution

or

intermediate_point = pos2 - bond_vector * atom2_contribution

atom1_contribution + atom2_contribution = r1 / (r1 + r2) + r2 / (r1 + r2) = (r1 + r2) / (r1 + r2) = 1

I'd appreciate it if you could take a moment to double-check our work before mentioning any concerns about the math. Thank you!

For the record, we discussed with @superstar54 and @cpignedoli, where we agreed to cut the bond at the middle point between two atoms

done in 9ee0f99

superstar54 · 2023-11-10T14:09:13Z

aiidalab_widgets_base/viewers.py

                self._viewer.set_representations(nglview_params, component=0)
                self._viewer.add_unitcell()
+                self._viewer._add_shape(set(bonds), name="bonds")


Even though it is not obvious in the GUI, but you add new shapes for the bonds, and haven't removed the default bond from the nglview itself, thus the two bonds overlap.

That was a hard nut to crash, but I think I managed in 066aa4a. What do you think @cpignedoli?

After setting different radii for different atoms in this commit, the wrong intermediate_point becomes obvious. Before, all atoms have the same radius in the ball+stick model, thus intermediate_point is (only) correct in that case.

I am not sure we do not remove default bonds: we never use the ngl representation "ball+stik" we always use spacefill (no bonds) and add bond shapes in the case we use the "fake ball+stik"

v1 + bond_vector * Radius[atom1.symbol] / ( Radius[atom1.symbol] + Radius[atom2.symbol]
bond_vector goes from atom1 to atom2.
we will add to atom1 a vector bond_v*r1/(r1+r2) and to atom2 a vector, in opposite direction (or from intermediate point to atom2), bond_v*r2/(r1+r2) in total: bond_v*(r1+r2)/(r1+r2) = bond_v and the two portions of bonds will have length proportional to the size of the spheres

unkcpz

Thanks a lot @yakutovicha. I only have some tiny requests in terms of code style, @superstar54 did a more thorough test.

.github/workflows/ci.yml

aiidalab_widgets_base/dicts.py

aiidalab_widgets_base/viewers.py

danielhollas

You already got two great reviews so I am leaving this one alone. :-)

Co-authored-by: Jusong Yu <[email protected]>

yakutovicha · 2023-11-16T00:15:39Z

@cpignedoli please comment on where all the numbers are coming from.

superstar54 · 2023-11-16T08:22:36Z

Hi @yakutovicha, Thanks for the update! I am still worried about the test on the bond.

This function is executed during text, you can check the codecov report, for instance.

It's executed, but the result isn't verified. I would suggest running this function and checking the result bonds using a simple structure as input, e.g., H2O. See my comment on the math to calculate the intermediate_point.

yakutovicha · 2023-11-16T16:57:00Z

I would suggest running this function and checking the result bonds using a simple structure as input, e.g., H2O.

Done in 9986465

unkcpz

My requests are all addressed, you have my approval. Thanks for the nice work! @yakutovicha

superstar54

Hi @yakutovicha, Thanks for the update. Since bond is an important component in the viewer, and this PR is supposed to handle large systems with > 1000 bonds. So I added a comment on the performance.

aiidalab_widgets_base/viewers.py

for more information, see https://pre-commit.ci

superstar54 · 2023-11-17T16:21:07Z

@unkcpz @yakutovicha I moved the if outside the for loop, which makes the code ~1 time faster. Please review.

unkcpz

Some small requests should be easy to address. @superstar54

unkcpz · 2023-11-17T16:32:41Z

aiidalab_widgets_base/viewers.py

+        bonds = []
+        if len(structure) <= 1:
+            return bonds


Suggested change

bonds = []

if len(structure) <= 1:

return bonds

if len(structure) <= 1:

return []

I think this is safe? No other place uses bonds, and all conditions return list.

unkcpz · 2023-11-17T16:34:07Z

aiidalab_widgets_base/viewers.py

@@ -554,9 +555,74 @@ def _observe_all_representations(self, change):
        if change["new"]:
            self._all_representations[-1].viewer_class = self

+    def _compute_bonds(self, structure, radius=1.0, color="element", povray=False):
+        """Create an array of bonds for the structure."""


"Create an array"? But the return value is a list.

unkcpz · 2023-11-17T16:38:03Z

aiidalab_widgets_base/viewers.py

+        v2 = v1 + bond_vectors * 0.5
+
+        # Choose the correct way for computing the cylinder.
+        def povray_cylinder(v1, v2, radius, color):


I agree using function makes it more clean for me. Can you move it out as staticmethod instead of nesting it here? If it is only used in the class, change the name to _povray_cylinder. Same for cylinder -> _cylinder and move it out.

Thanks, I changed the inner function into a private helper function.

aiidalab_widgets_base/viewers.py

superstar54 · 2023-11-17T20:08:49Z

Hi @unkcpz, thanks for the review. I made the changes as you suggested. Please have another look.

unkcpz

Thanks! Looks all good. The pr is waiting for your approve @superstar54 😄

Implementing bonds computed via ase.

ec2ba9d

yakutovicha added the enhancement New feature or request label Nov 6, 2023

yakutovicha added 3 commits November 7, 2023 13:25

Merge branch 'master' into feature/draw-atomic-bonds

05d90c3

working version

abb240a

Cosmetic changes.

c66fc3a

yakutovicha added 2 commits November 7, 2023 16:25

Remove unnecessary code.

942539f

Work on compute_bonds method.

ef69ba0

yakutovicha marked this pull request as draft November 7, 2023 23:37

yakutovicha added 10 commits November 8, 2023 11:12

Remove unnecessary function update_representations

57380c8

Remove unnecessary representation_parameters method.

349ef64

Move _on_atom_click, highlight_atoms, remove_viewer_components back w…

6eeef88

…here they were.

Remove unnecessary code.

7d7c2d7

Simplify _compute_bonds method.

4a978ab

Fix tests.

32c08e1

Add tests for the povray rendering function.

360aba7

Merge branch 'master' into feature/draw-atomic-bonds

464b5ed

Install povray

73907b3

Merge branch 'feature/draw-atomic-bonds' of https://github.com/aiidal…

5aa629c

…ab/aiidalab-widgets-base into feature/draw-atomic-bonds

yakutovicha requested review from danielhollas, cpignedoli and superstar54 November 9, 2023 08:56

yakutovicha marked this pull request as ready for review November 9, 2023 08:56

yakutovicha added this to the v2.1.0 milestone Nov 9, 2023

superstar54 requested changes Nov 10, 2023

View reviewed changes

unkcpz requested changes Nov 10, 2023

View reviewed changes

.github/workflows/ci.yml Show resolved Hide resolved

aiidalab_widgets_base/dicts.py Outdated Show resolved Hide resolved

aiidalab_widgets_base/viewers.py Outdated Show resolved Hide resolved

aiidalab_widgets_base/viewers.py Show resolved Hide resolved

danielhollas reviewed Nov 10, 2023

View reviewed changes

yakutovicha and others added 2 commits November 15, 2023 22:10

work on RGB_COLORS.

011ec6e

Update aiidalab_widgets_base/viewers.py

c83c196

Co-authored-by: Jusong Yu <[email protected]>

yakutovicha added 2 commits November 16, 2023 14:17

atoms_in_representation method: fix typo, update function.

fc815dc

Merge branch 'master' into feature/draw-atomic-bonds

a5fdfb8

yakutovicha force-pushed the feature/draw-atomic-bonds branch from e03ec73 to a5fdfb8 Compare November 16, 2023 15:47

yakutovicha added 3 commits November 16, 2023 16:47

Merge branch 'master' into feature/draw-atomic-bonds

67573e2

Change bond color at the middle point between two atoms.

9ee0f99

Simplify size multipliers and add comments.

6e547fc

Add test for the bonds.

9986465

yakutovicha force-pushed the feature/draw-atomic-bonds branch from d780b5a to 9986465 Compare November 16, 2023 17:46

yakutovicha requested review from superstar54 and unkcpz November 16, 2023 17:47

unkcpz previously approved these changes Nov 16, 2023

View reviewed changes

superstar54 requested changes Nov 17, 2023

View reviewed changes

aiidalab_widgets_base/viewers.py Outdated Show resolved Hide resolved

aiidalab_widgets_base/viewers.py Outdated Show resolved Hide resolved

aiidalab_widgets_base/viewers.py Outdated Show resolved Hide resolved

improve performance

3b50545

superstar54 dismissed unkcpz’s stale review via 3b50545 November 17, 2023 15:02

pre-commit-ci bot and others added 2 commits November 17, 2023 15:03

[pre-commit.ci] auto fixes from pre-commit.com hooks

0374820

for more information, see https://pre-commit.ci

use function

cfb9f2a

superstar54 requested a review from unkcpz November 17, 2023 16:18

unkcpz requested changes Nov 17, 2023

View reviewed changes

change inner function into private helper function

0cd8708

unkcpz approved these changes Nov 17, 2023

View reviewed changes

superstar54 self-requested a review November 17, 2023 20:58

superstar54 approved these changes Nov 17, 2023

View reviewed changes

unkcpz merged commit ca8322d into master Nov 17, 2023
13 checks passed

unkcpz deleted the feature/draw-atomic-bonds branch November 17, 2023 23:02

superstar54 mentioned this pull request Mar 5, 2024

mimic the behavior of nglview and update the cell and positions #576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draw bonds computed by ASE #535

Draw bonds computed by ASE #535

yakutovicha commented Nov 6, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

yakutovicha commented Nov 9, 2023

superstar54 left a comment

superstar54 Nov 10, 2023

cpignedoli Nov 16, 2023

yakutovicha Nov 16, 2023

superstar54 Nov 10, 2023

yakutovicha Nov 15, 2023

superstar54 Nov 16, 2023 •

edited

Loading

yakutovicha Nov 16, 2023 •

edited

Loading

yakutovicha Nov 16, 2023 •

edited

Loading

yakutovicha Nov 16, 2023

superstar54 Nov 10, 2023

yakutovicha Nov 16, 2023

superstar54 Nov 16, 2023

cpignedoli Nov 16, 2023

cpignedoli Nov 16, 2023 •

edited

Loading

unkcpz left a comment

danielhollas left a comment

yakutovicha commented Nov 16, 2023

superstar54 commented Nov 16, 2023

yakutovicha commented Nov 16, 2023 •

edited

Loading

unkcpz left a comment

superstar54 left a comment

superstar54 commented Nov 17, 2023 •

edited

Loading

unkcpz left a comment

unkcpz Nov 17, 2023 •

edited

Loading

unkcpz Nov 17, 2023 •

edited

Loading

unkcpz Nov 17, 2023

superstar54 Nov 17, 2023

superstar54 commented Nov 17, 2023

unkcpz left a comment •

edited

Loading


		radius = radius / 5

		cutoff = ase.neighborlist.natural_cutoffs(structure, mult=1.09)

Draw bonds computed by ASE #535

Draw bonds computed by ASE #535

Conversation

yakutovicha commented Nov 6, 2023

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

yakutovicha commented Nov 9, 2023

superstar54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

yakutovicha Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

yakutovicha Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpignedoli Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

danielhollas left a comment

Choose a reason for hiding this comment

yakutovicha commented Nov 16, 2023

superstar54 commented Nov 16, 2023

yakutovicha commented Nov 16, 2023 • edited Loading

unkcpz left a comment

Choose a reason for hiding this comment

superstar54 left a comment

Choose a reason for hiding this comment

superstar54 commented Nov 17, 2023 • edited Loading

unkcpz left a comment

Choose a reason for hiding this comment

unkcpz Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

unkcpz Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 commented Nov 17, 2023

unkcpz left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 7, 2023 •

edited

Loading

superstar54 Nov 16, 2023 •

edited

Loading

yakutovicha Nov 16, 2023 •

edited

Loading

yakutovicha Nov 16, 2023 •

edited

Loading

cpignedoli Nov 16, 2023 •

edited

Loading

yakutovicha commented Nov 16, 2023 •

edited

Loading

superstar54 commented Nov 17, 2023 •

edited

Loading

unkcpz Nov 17, 2023 •

edited

Loading

unkcpz Nov 17, 2023 •

edited

Loading

unkcpz left a comment •

edited

Loading