use displayed_structure to create selection info #371

superstar54 · 2022-10-18T13:34:42Z

fixes #361
fixes #413

Since selection can only be applied to the displayed_structure, I replaced the self.structure by self.displayed_structure.

yakutovicha

Hey @superstar54, I think there is an issue with this approach.

The displayed_structure does not actually correspond to the "real" structure. So does the selection of atoms, etc.

One needs to make sure that the indexes selected for the displayed_structure are mapped to the structure and vice-versa.

That is the most complicated part here (I think).

superstar54 · 2022-10-18T14:40:30Z

Yes, this is the tricky part. In the case of the supercell in Appearance tab, this map relation is fulfilled automatically because of the ase.Atoms.repeat method is used.

self.set_trait("displayed_structure", self.structure.repeat(self.supercell))

In this ase.Atoms.repeat method (https://wiki.fysik.dtu.dk/ase/_modules/ase/atoms.html#Atoms.repeat), the numpy.tile function is used to repeat the atoms.arrays. Suppose there are N atoms in the "real" structure, and there are N*M atoms in the repeated structure. The first N atoms in the repeated structure are exactly the same atoms in the "real" structure. Therefore, if the user selects the atoms in the range 0 ~ N, the index is correct. If the user selects atoms index >= N, then the "virtual" atoms are selected, and the selection info is correct too, even though it's meaningless for DFT calculation.

yakutovicha · 2022-10-20T16:01:50Z

If the user selects atoms index >= N, then the "virtual" atoms are selected, and the selection info is correct too, even though it's meaningless for DFT calculation.

Unfortunately, this is not always good. Because structure editors typically operate on the real atoms, not the virtual ones. So if you call an atom deletion, you don't want to attempt deleting the virtual atoms. Otherwise, things will crash.

However, I see the need to compute the distance (angles, dihedrals) across the replicas. This can be done by operating on the virtual atoms (your approach).

A possible solution would be to have two selections: displayed_selection and selection that operate on displayed_structure and structure traits.

@cpignedoli implemented an alternative approach in #373. I asked him to remove it from the PR since the "right way" doesn't look obvious.

superstar54 · 2022-10-20T16:36:43Z

Two selections are not necessary, because the selection is the subset of the displayed_selection.

Another solution could be adding the validation of the selection for any operation on the structure. Add a warning message to alert users that they are trying to delete virtual atoms. Because the Appearance tab is only for visualization. If they want to create a supercell and delete atoms, they need to go to the Edit Cell section instead of Appearance.

This applies to the displayed_structure too, e.g. highlight_atoms. Currently, inputting the wrong index in the selection textbox and clicking the apply button will crash the app too.

The validation is straightforward, the selection should be a subset of the range(0, N). N is the number of atoms for structure or displayed_structure.

yakutovicha · 2022-10-21T08:16:02Z

Two selections are not necessary, because the selection is the subset of the displayed_selection.

I don't think this is true in this particular case, nor in general. The reason to go from a data object to a subset of data could be simply due to convenience.

See my reasoning below for this particular case.

Another solution could be adding the validation of the selection for any operation on the structure. Add a warning message to alert users that they are trying to delete virtual atoms. Because the Appearance tab is only for visualization. If they want to create a supercell and delete atoms, they need to go to the Edit Cell section instead of Appearance.

Why add this validation to the editor, if this can be done once in the viewer? I think it makes more sense that the viewer already provides the correct selection. In the current approach, one could add an arbitrary number of editors, so this is not clear to me why every one of those should handle the selection if they are not supposed to act on the virtual atoms.

This applies to the displayed_structure too, e.g. highlight_atoms. Currently, inputting the wrong index in the selection textbox and clicking the apply button will crash the app too.

This we must fix.

The validation is straightforward, the selection should be a subset of the range(0, N). N is the number of atoms for structure or displayed_structure.

I agree that the validation is straightforward, I just don't understand why you want to pass the displayed_selection to every editor and repeat the same slicing operation for each of them.

superstar54 · 2022-10-21T11:06:02Z

I think it makes more sense that the viewer already provides the correct selection

Good point! I will implement validation in this way.

yakutovicha · 2022-11-15T14:25:16Z

@cpignedoli, @superstar54 and I discussed the selection handling. The draft schema below is what we've agreed on:

cpignedoli · 2022-11-16T21:49:24Z

I slightly changed my mind in part recovering what @yakutovicha was suggesting. Please @superstar54 and @yakutovicha consider this:
to have complete info and non-redundant info at the same time
we need to know

to which atom in the unit cell does a supercell atom corresponds
to have the unique is of clicked atoms

Thus I propose updating the skecth that we discussed as follows
The light red circles highlight the atoms that I clicked

This allows me to make a selection in the sometimes more intuitive supercell representation
and e.g. delete the correct atoms from the real unit cell

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

superstar54 · 2022-11-22T13:07:55Z

Here are the changes:

replay old selection with displayed_selection.
the new selection is generated by selection = [x for x in displayed_selection if x <= self.natom], thus the self.selection is always correct for Structure Editors.
add one information line for Unit cell atoms.
the unit_cell_selection atoms is calculated by {x % self.natom + 1 for x in displayed_selection}

I implemented what @cpignedoli suggested. However, the line for Selected atoms is not necessary because it duplicates with the top level Selected atoms.

Here is the screenshot:

cpignedoli · 2022-11-22T13:58:57Z

I agree we just have to decide how to pass the "unit cell atoms" set to the editors

yakutovicha

Thanks a lot @superstar54. A few comments:

I thought we agreed to not convert the field where we type in formulas to the list of indices. That could simplify some logic. Also, some users complained that they were not happy with the disappearance of the formula.
Related to the previous: I think we should display the displayed_selection along with unit cell atoms.

aiidalab_widgets_base/viewers.py

yakutovicha · 2022-12-12T08:20:31Z

@superstar54 any progress here? Let me know if I should take over this PR.

superstar54 · 2022-12-12T08:22:46Z

@superstar54 any progress here? Let me know if I should take over this PR.

Last two weeks have been very busy. I will work on this today.

remove wrong_syntax line Co-authored-by: Aliaksandr Yakutovich <[email protected]>

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

yakutovicha · 2022-12-15T12:45:03Z

Hi all,

just a general comment, please make sure everything works for gas phase molecules as well. No information about cell should be displayed when the structure is not periodic. 😊

@danielhollas I can reassure you that we do not touch the handling cells. It is only about atom selection.

yakutovicha · 2022-12-15T16:42:37Z

I must admit that structure multiplication is not working as expected in the tests. It seems that the button is not always clicked. Instead of clicking, one should put a number there (2 in this case).

cpignedoli

There is a problem:
after an edit operation the list of selected atoms /display atoms, should be updated according to what is defined in structures.py in the corresponding edit function. For example, after "copy" of a selection, the user should see highlighted automatically the newly added atoms. This is crucial to be able afterward to move correctly and easily the atoms. This is not the case anymore: after an edit operation, the selected /display atoms are reset.

Also, add "sleep" before pressing "Apply selection". Otherwise, the button is oftern pressed too fast.

yakutovicha · 2023-01-05T16:54:10Z

@cpignedoli, your concern was addressed in f2d5f19. Please test it again and let me know if everything works as expected.

cpignedoli

Everything I tested was working.
Please in replace "Selected atoms" with "Select atoms" in the field where we enter the selection (e.g. x>2)

tests/test_notebooks.py

yakutovicha · 2023-01-09T17:07:57Z

Everything I tested was working. Please in replace "Selected atoms" with "Select atoms" in the field where we enter the selection (e.g. x>2)

done in a2fd1e7

danielhollas · 2023-01-26T16:51:09Z

aiidalab_widgets_base/viewers.py


-        # if atom is selected from nglview, shift to selection tab


@yakutovicha why was this code deleted?

I don't remember the exact motivation. I think it is because the trait doesn't have to be set by nglview. If it is set elsewhere, the switching will still occur, which might be unwanted.

Ok, that makes sense. Thanks!

…ption (#429) PR #371 broke the `configuration_tabs` option of the `StructureDataViewer`. In the latest release, no configuration tabs are displayed when this option is passed to the class constructor. Co-authored-by: Aliaksandr Yakutovich <[email protected]>

use displayed_structure to create selection info

6650466

superstar54 requested a review from yakutovicha October 18, 2022 13:40

yakutovicha requested changes Oct 18, 2022

View reviewed changes

yakutovicha mentioned this pull request Oct 24, 2022

Implement viewer representations #373

Merged

superstar54 and others added 5 commits November 22, 2022 10:50

Merge branch 'master' into fix_supercell_selection

720ae1f

add info_unit_cell_atoms

2b369ab

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

9bac99b

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

add displayed_selection

e05a28e

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

fd8c279

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

superstar54 requested a review from yakutovicha November 22, 2022 13:09

yakutovicha requested changes Nov 29, 2022

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

aiidalab_widgets_base/viewers.py Outdated Show resolved Hide resolved

yakutovicha mentioned this pull request Dec 12, 2022

StructureViewer crashes when Selection is out of bounds #413

Closed

superstar54 and others added 7 commits December 12, 2022 10:00

Merge branch 'master' into fix_supercell_selection

129fb82

Update aiidalab_widgets_base/viewers.py

94e30b7

remove wrong_syntax line Co-authored-by: Aliaksandr Yakutovich <[email protected]>

Update aiidalab_widgets_base/viewers.py

8b3f5e4

remove wrong_syntax line Co-authored-by: Aliaksandr Yakutovich <[email protected]>

do not update selection formula

a68ac08

add a bulk pt for test

8f13226

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

f90f8b4

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

use try for advance parse_advanced_sel

a157ab8

fix test

ee21af9

yakutovicha added 4 commits December 15, 2022 13:06

Trying to test supercell selection

b50e84c

fix

48d7849

Fix tests

a13d260

Revert changes to the tests

746b603

cpignedoli requested changes Dec 16, 2022

View reviewed changes

yakutovicha added 3 commits December 19, 2022 14:31

(tests) More robust cell expansion in z-direction.

8505ea6

Slightly increase the size of supercell selection widgets

9b6fc31

Also, add "sleep" before pressing "Apply selection". Otherwise, the button is oftern pressed too fast.

Introduce 'input_selection' trait to interact with editors

f2d5f19

yakutovicha requested a review from cpignedoli January 5, 2023 16:54

cpignedoli previously approved these changes Jan 9, 2023

View reviewed changes

Merge branch 'master' into fix_supercell_selection

d4fccda

yakutovicha dismissed cpignedoli’s stale review via d4fccda January 9, 2023 16:10

Fix pre-commit

b998203

yakutovicha reviewed Jan 9, 2023

View reviewed changes

tests/test_notebooks.py Outdated Show resolved Hide resolved

Update tests/test_notebooks.py

58b641a

yakutovicha self-requested a review January 9, 2023 16:26

Selected atoms -> Select atoms.

a2fd1e7

Fix tests

010c899

yakutovicha approved these changes Jan 9, 2023

View reviewed changes

yakutovicha merged commit 0861b83 into master Jan 9, 2023

yakutovicha deleted the fix_supercell_selection branch January 9, 2023 18:11

danielhollas reviewed Jan 26, 2023

View reviewed changes

danielhollas mentioned this pull request Jan 26, 2023

configuration_tabs option of StructureDataViewer is broken #425

Closed

danielhollas mentioned this pull request Feb 13, 2024

ASE deprecation warnings #387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use displayed_structure to create selection info #371

use displayed_structure to create selection info #371

superstar54 commented Oct 18, 2022 •

edited by yakutovicha

Loading

yakutovicha left a comment

superstar54 commented Oct 18, 2022 •

edited

Loading

yakutovicha commented Oct 20, 2022 •

edited

Loading

superstar54 commented Oct 20, 2022

yakutovicha commented Oct 21, 2022 •

edited

Loading

superstar54 commented Oct 21, 2022

yakutovicha commented Nov 15, 2022

cpignedoli commented Nov 16, 2022 •

edited

Loading

superstar54 commented Nov 22, 2022 •

edited

Loading

cpignedoli commented Nov 22, 2022

yakutovicha left a comment

yakutovicha commented Dec 12, 2022

superstar54 commented Dec 12, 2022

yakutovicha commented Dec 15, 2022 •

edited

Loading

yakutovicha commented Dec 15, 2022

cpignedoli left a comment

yakutovicha commented Jan 5, 2023

cpignedoli left a comment

yakutovicha commented Jan 9, 2023

danielhollas Jan 26, 2023

yakutovicha Jan 30, 2023

danielhollas Jan 30, 2023

use displayed_structure to create selection info #371

use displayed_structure to create selection info #371

Conversation

superstar54 commented Oct 18, 2022 • edited by yakutovicha Loading

yakutovicha left a comment

Choose a reason for hiding this comment

superstar54 commented Oct 18, 2022 • edited Loading

yakutovicha commented Oct 20, 2022 • edited Loading

superstar54 commented Oct 20, 2022

yakutovicha commented Oct 21, 2022 • edited Loading

superstar54 commented Oct 21, 2022

yakutovicha commented Nov 15, 2022

cpignedoli commented Nov 16, 2022 • edited Loading

superstar54 commented Nov 22, 2022 • edited Loading

cpignedoli commented Nov 22, 2022

yakutovicha left a comment

Choose a reason for hiding this comment

yakutovicha commented Dec 12, 2022

superstar54 commented Dec 12, 2022

yakutovicha commented Dec 15, 2022 • edited Loading

yakutovicha commented Dec 15, 2022

cpignedoli left a comment

Choose a reason for hiding this comment

yakutovicha commented Jan 5, 2023

cpignedoli left a comment

Choose a reason for hiding this comment

yakutovicha commented Jan 9, 2023

danielhollas Jan 26, 2023

Choose a reason for hiding this comment

yakutovicha Jan 30, 2023

Choose a reason for hiding this comment

danielhollas Jan 30, 2023

Choose a reason for hiding this comment

superstar54 commented Oct 18, 2022 •

edited by yakutovicha

Loading

superstar54 commented Oct 18, 2022 •

edited

Loading

yakutovicha commented Oct 20, 2022 •

edited

Loading

yakutovicha commented Oct 21, 2022 •

edited

Loading

cpignedoli commented Nov 16, 2022 •

edited

Loading

superstar54 commented Nov 22, 2022 •

edited

Loading

yakutovicha commented Dec 15, 2022 •

edited

Loading