aiidalab / aiidalab-widgets-base

Reusable widgets for AiiDAlab applications
MIT License
7 stars 17 forks source link

Draw bonds computed by ASE #535

Closed yakutovicha closed 10 months ago

yakutovicha commented 11 months ago

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

image (15)

To Do:

codecov[bot] commented 11 months ago

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 :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/535/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | Coverage Δ | | |---|---|---| | [python-3.10](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/535/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `87.12% <98.57%> (+1.23%)` | :arrow_up: | | [python-3.8](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/535/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `87.15% <98.57%> (+1.23%)` | :arrow_up: | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/535/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `87.15% <98.57%> (+1.23%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

yakutovicha commented 11 months ago

[ ] 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.

yakutovicha commented 10 months ago
  • better to add a test for the _compute_bonds function.

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

  • 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.

Done in https://github.com/aiidalab/aiidalab-widgets-base/pull/535/commits/289139c9c49a32a6899bc6adb5668c30f9094a3b, thanks!

unkcpz commented 10 months ago

I guess test failed because of dockerhub rate limit, https://github.com/aiidalab/aiidalab-widgets-base/pull/539 should fix it.

yakutovicha commented 10 months ago

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

superstar54 commented 10 months ago

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 commented 10 months ago

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

Done in https://github.com/aiidalab/aiidalab-widgets-base/pull/535/commits/9986465d404773457e24a81dcf4a9df16afb60f2

superstar54 commented 10 months ago

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

superstar54 commented 10 months ago

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