Implement viewer representations

[cpignedoli]

fixes #366

We have to take an important design decision before we continue on this:
I tried to have some concepts inherited from other viewers, especially:
if I have a structure and I define for it a couple of representations (e.g. a slab with VDW radii and a molecule on the slab as ball+stick) it is crucial that when I modify the structure (e.g. removing some atoms) my representations remain active and properly working.

This requires that in the editors we keep track of the defined representations,

https://github.com/cpignedoli/aiidalab-widgets-base/blob/3d642d16cb291c308d0614d1f302d7885b4cf723/aiidalab_widgets_base/structures.py#L1516

not a big deal but an important addition one should not forget. Do we agree on this? is there a better design for this concept?
(the code above avoid, e.g., taht if I rmeove atoms form a structure (see image) teh other atoms will lose theyr correct representation)

Also, I would like to avoid redrawing a representation unnecessarily but this seems to be difficult...

At the moment update of the viewer is triggered:
-if a structure is imported
-if a structure is modified
-if a user changes the representations and clicks on "apply representation"

[superstar54]

@cpignedoli Thanks for implementing this. The concept is OK to me.

[danielhollas]

Hi @cpignedoli,

Thanks for the PR and work. I think all of this also ties into our previous discussing with @yakutovicha and @unkcpz about the need to refactor the StructureViewer classes, please take a look at the issue #342. I am afraid that we should first solve that issue, because here you're adding a lot of new code to an already overloaded class, which would make it even harder to refactor later.

I would also like to raise the question about the scope of this widget. Do we really need to support different representations for different part of the system? That seems to me like a big complication. Are we trying to essentially implement a full-fledged molecular editor here, or should we limit ourselves to make things simple. It would help if you could write a bit more about your use case.

By the way, would you mind providing some screenshots of how the interface looks like right now?

In general, it definitely seems weird to have editors care about the representation.

EDIT: Apologies if these things were already discussed in today's meeting and I am out of the loop...

[cpignedoli]

Hi @danielhollas
I fully see your point and I had discussions with @yakutovicha who shares your concerns.

From the perspective of a user, I can imagine a viewer without an editor (for example VMD, an excellent viewer that is not really meant for editing) but I cannot imagine an editor without a viewer, you should see what you edit and in many cases, you do not want to edit parts of a system that you decided to hide.

This creates necessarily a two-way communication between the editor and the viewer.

The most simple implementation I am able to imagine for this (not the current implementation) is that the viewer and the editors work on e.g. an ase geometry that has an additional channel (e.g. an integer but only 'tag' would be available) indicating to which representation an atom belongs. The viewer knows what to do out of that integer.

As soon as a geometry is imported a default value is assigned for the representation, if representations are created the 'tags' are modified

Coming back to why as a user I am strongly in favor of having the possibility of multiple representations;
I think I never managed to work without:
in the snapshot below you can see that already the simple task of recognizing the stacking mode in a surface reconstruction (VMD example for reconstructed Au(111) requires hiding some layers and highlighting others in different ways.

If it comes to modifying a geometry before running a simulation, I provide an example of a molecule on gold:
The standard AiiDAlab viewer offers a perspective that is almost useless I even cannot identify easily an adatom that I may want to move below a phenyl ring.

Having two representations enhances enormously the possibility to edit promptly the structure.

Not to speak about simulaitons where you wan tto partition a system in a QM and an MM part, it oculd well be you need to hide or simplyfy teh representation of the atom sbelonging to the MM part.

Having two representations enhances enormously the possibility to edit promptly the structure.

In terms of complexity, let me recall that, at the viewer level (with nglview), dealing with one or several representations means
going back and forth between two dictionaries

def _gen_translation_indexes(self):
     """Transfromation of indexes in case of multiple representations
      dictionaries for  back and forth transformations.
      suppose we have 3 representations:
      component = 0,1,2
      and a structure with 10 atoms.
      If we assign the first 3 atoms to the first representation
      atom 4 to the second and the rest to the third
      the two dictionaries will look like:
      {0:(0,0),1:(0,1),2:(0,2),3:(1,0),4:(2,0),5:(2,1),6:(2,2),7:(2,3),8:(2,4),9:(2,5)}
      and
      {(0,0):0,(0,1):1,(0,2):2,(1,0):3,(2,0):4,(2,1):5,(2,2):6,(2,3):7,(2,4):8,(2,5):9}
   """

I am available for a zoom call with all of you to discuss further a possible route to create a meaningful viewer.

P.S. the version I drafted (at least before the latest bugs I introduced) handles supercells (from the appearance tab) and if an atom from a replica is selected, the corresponding atom in the "principal unit" is highlighted. Distances are computed within the unit not within replicas. This of course could be changed in favor of MIC convention

[yakutovicha]

We discussed this with @cpignedoli and possibly came up with a better solution. It should satisfy both: the ability to represent atoms differently (as expressed by @cpignedoli) and our strive for simplicity (as highlighted by @danielhollas).

It is true, that passing representation to the editors is a direct violation of the separation of concerns concept. So I am fully on @danielhollas's side here.

However, there is one trick that we can do to avoid explicitly passing this information to the editors: store it internally in the ase.Atoms object as a representation array. Here is how this can be achieved:

from ase.build import molecule
import numpy as np
m = molecule('H2O')
m.cell = [5,5,5]
m.set_array('representations', np.array([0, 1, 0]))  # <-- each integer corresponds to a different representation.
m.get_array('representations')

The advantage of this approach is that the representation array is automatically (and correctly!) handled along all the transformations:

The only disadvantage of the current approach is that it doesn't handle the same atom in different representations. But this could be resolved by creating a separate array for every representation, something like representation_0, representation_1, etc...

We agreed that @cpignedoli will further develop this concept.

[yakutovicha]

@danielhollas, @unkcpz Thanks for the review. I addressed all the changes or replied to your comments about why I couldn't do certain things.

Please have a look. Also, if things can be postponed to the follow-up PR (which is #377), I would appreciate it. This one is already quite big and quite thoroughly tested by @cpignedoli.

For the documentation build failing - don't worry about it. I will fix it before I merge the PR.

[yakutovicha]

I have to say that I dislike those random sphinx failures so much... From day to night the documentation build suddenly starts to fail, and then one needs to spend time understanding what has happened.

[yakutovicha]

@cpignedoli, @danielhollas, @superstar54, I hope I addressed your comments. Is there anything else you want me to add to this PR?

[danielhollas]

@yakutovicha sorry for the delay, I got bog down with other. Still wanted to get a second pass on the review if that is okay. Will work on it now I hope, if the connection in the train is decent.

[danielhollas]

@yakutovicha here's what I managed to get through for now. Besides some minor or optional things, there is one important change about the failing test that I think needs to be reverted,

[danielhollas]

@yakutovicha I don't think your solution in that commit is correct (it looks like a breaking change possibly). input_structure cannot be passed via the constructor. I think you just need to revert the change in this test.
Unless I am missing something.

[danielhollas]

For this, you need to create an Enum that is further converted to a tuple, so why not just create a tuple at first?

Because by using Enum you have a single source of truth, and you can then use it elsewhere when you have code that depends on the options (instead of comparing strings). In other words using a type is nicer than using strings: https://www.lesinskis.com/stringly-typed-functions.html

But agree, let's not derail this here. :-D

[yakutovicha]

@danielhollas, I believe I addressed all your suggestions in one way or another. Please have a look.

And thank you very much for the careful review. I appreciate your time and effort.

[yakutovicha]

During tomorrow's documentation day, I will draw a data-flow schema and will add it to the documentation. This will clarify a lot of things.

[yakutovicha]

@danielhollas, do you want to have another look at this PR? It would be great to close this story to move on with other things.

[danielhollas]

LGTM, thanks for implementing the changes @yakutovicha. I wanted to do a quick test in my app, but realized that I hide the Appearence tab to simplify the UI. 😄

Excited to get this big feature merged, congrats! 🎉 🚀

During tomorrow's documentation day, I will draw a data-flow schema and will add it to the documentation. This will clarify a lot of things.

This is an awesome idea! Did you end up doing that? If so, could you please post it here as well? In general I think we should do that for all widgets that are a bit more complicated (all the widgets around the structure / viewer for example).

And thank you very much for the careful review. I appreciate your time and effort.

❤️

...I have to admit there is nothing to nitpick, it is quite clear what the PR wants to achieve with nice unit tests. I nitpick on one unit test to add an assertion.

I guess I was the "reviewer number 2" for this PR 😊 😂

[yakutovicha]

I guess I was the "reviewer number 2" for this PR 😊 😂

technically, I started looking into this PR as a reviewer too 😄

[yakutovicha]

I can even approve 😎

[yakutovicha]

This is an awesome idea! Did you end up doing that? If so, could you please post it here as well? In general I think we should do that for all widgets that are a bit more complicated (all the widgets around the structure / viewer for example).

I didn't manage to finish that task, but I added an issue to keep in mind #534.

[unkcpz]

Congrats! @yakutovicha 🥂

[unkcpz]

Put my green mark here as well ;)