search

aiidalab / aiidalab-widgets-base

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

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

Closed superstar54 closed 5 months ago

superstar54 commented 7 months ago

Fix #575

After inspecting the source code of nglview, I found that nglview loads the structure through saves and loads a temp "pdb" file. This process could change the cell and positions of the structure.

This cause problems

This PR fixes the second problem. I transferred the displayed_structure into a standard form, with the a axes aligned with the X axis, so that the displayer_structure will not be rotated anymore when saving to a pdb file. Thus the viewer will show the correct displayed_structure. In this case, the calculated bonds will match the atoms.

danielhollas commented 7 months ago

After inspecting the source code of nglview, I found that nglview loads the structure through saves and loads a temp "pdb" file.

Ooof, that's horrifying. Can you please clarify when is this happening? Is it happening for any kind of structure (e.g. even without a cell?)? Or only for in certain cases?

superstar54 commented 7 months ago

Ooof, that's horrifying. Can you please clarify when is this happening? Is it happening for any kind of structure (e.g. even without a cell?)? Or only for in certain cases?

The ASEStructure uses it, which, unfortunately, means it is used for all structures inside AiiDAlab https://github.com/nglviewer/nglview/blob/12a75bc6f0cea8fbc0f75ffc68117d010ce96cc9/nglview/adaptor.py#L137

load structure into nglview: https://github.com/nglviewer/nglview/blob/12a75bc6f0cea8fbc0f75ffc68117d010ce96cc9/nglview/widget.py#L1241

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.17%. Comparing base (826ad43) to head (1c57555). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #576 +/- ## ======================================= Coverage 96.17% 96.17% ======================================= Files 11 11 Lines 1177 1177 ======================================= Hits 1132 1132 Misses 45 45 ``` | [Flag](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/576/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/576/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `96.17% <ø> (ø)` | | | [python-3.9](https://app.codecov.io/gh/aiidalab/aiidalab-widgets-base/pull/576/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidalab) | `96.17% <ø> (ø)` | | 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.

danielhollas commented 7 months ago

Was this a problem in version 2.0, or has this been introduced with the new features in 2.1. This workaround seems really icky.

The ASEStructure uses it, which, unfortunately, means it is used for all structures inside AiiDAlab

If that is the case, why is this fix only needed when the supercell changes?

superstar54 commented 7 months ago

Was this a problem in version 2.0, or has this been introduced with the new features in 2.1.

This has existed from the very beginning (before awb 1.0). But it is not a problem, because user will not notice if both cell and position are rotated. And we are not exporting the rotated structure from the view. However, it becomes a problem when we draw bonds manually #535 , because we use the original structure to calculate the bonds.

If that is the case, why is this fix only needed when the supercell changes?

Good point. This is the only entry point where we set the structure in the viewer. One triggers this function when there is a new structure. It's not a good design, and can be improved in another PR.

danielhollas commented 7 months ago

This is the only entry point where we set the structure in the viewer.

Isn't that rather the structure traitlet? Confusing indeed, but okay for now.

TBH I am still not very happy with the solution here....We're writing a file every time a structure is updated, that's not going to help performance. Is there a way to somehow get the correct coordinates from the nglview (even if we needed to access its internal state), and pass them around to whenever we compute the bonds? That seems like a cleaner design and makes more sense in terms of data flow.

superstar54 commented 7 months ago

Isn't that rather the structure traitlet? Confusing indeed, but okay for now.

The displayed_structure is used in the nglview. https://github.com/aiidalab/aiidalab-widgets-base/blob/8190dc08ab9a25bae84a991cad165a1ff3d05d1d/aiidalab_widgets_base/viewers.py#L1197

TBH I am still not very happy with the solution here....We're writing a file every time a structure is updated, that's not going to help performance. Is there a way to somehow get the correct coordinates from the nglview (even if we needed to access its internal state), and pass them around to whenever we compute the bonds?

Good point. I didn't find a way to get data from nglview. But I manager to get the correct cell and positions by rotating the cell into a standard form.

danielhollas commented 7 months ago

I am a little bit scared of converting back and forth from pdb in case coordinates could lose accuracy but the tests I did seem to have no accuracy problems.

I agree that it is extremely fishy, but note that this conversion only happens inside of nglview, so it only affects what you see on the screen, not the data that we have internally in aiidalab widgets.

I wanted to update the files of the examples but I will do it on a separate PR as soon as I manage

Why do you want to update the examples?

cpignedoli commented 7 months ago

Why do you want to update the examples? We are trying to have a demo server as clean as possible, starting from examples where lattice parameters are all, for example, exp ones, the cell is primitive with symmetric axes settings, the name is clear (e.g. alpha/beta-quartz in place of SiO2)

we have a preliminary list of issues we would need to fix, I will share with you a google document

it only affects what you see on the screen, not the data that we have internally in aiidalab widgets.

indeed all tests I did show no issues, if I download a structure after doing back and forth from primitive to conventional, the coordinates are translated+rotated,wrt teh initial input geo, to the very last digit

superstar54 commented 5 months ago

Hi @danielhollas , thanks for the review! I made the changes as you suggested. Could you please take another look?

danielhollas commented 5 months ago

@superstar54 this feels like an important bugfix and seems like the issue is present in version 2.1.0. Should we backport it to 2.1.x branch? And release a bugfix version?

superstar54 commented 5 months ago

Hi @danielhollas , I'm not very familiar with release management, but if you believe it is good to do this, please go ahead with the necessary steps. Thanks for handling this!