Enhance cell editor #372

superstar54 · 2022-10-19T05:37:36Z

fixes #365

editing cell vectors directly.
cell transformation using a 3x4 transformation matrix.

unkcpz · 2022-10-21T10:54:38Z

@superstar54 Thanks!
That "Cell vectors" and "Cell transformation" duplicate each other? Why not just keep the "Cell transformation"?
I would suggest adding instructions on how the matrix apply to the cell.

superstar54 · 2022-10-21T11:14:24Z

That "Cell vectors" and "Cell transformation" duplicate each other?

No. The "Cell vectors" are the cell itself (3x3 array). This enables the user to edit the cell directly, e.g. cell[2][2] += 10, which will add a vacuum in the z-direction, making a surface slab.
The "Cell transformation" transforms the cell by multiplying a (3x4) matrix.

I would suggest adding instructions on how the matrix apply to the cell.

Agree. I will add a tooltip for every button.

unkcpz · 2022-10-21T11:39:59Z

This enables the user to edit the cell directly, e.g. cell[2][2] += 10, which will add a vacuum in the z-direction, making a surface slab.

I see, thanks for the explanation, this is interesting. But don't you think it is mind-binding to set the off-diagonal for changing the cell in this way?

Agree. I will add a tooltip for every button.

I think that is not enough, my suggestion is for each matrix you keep the matrix on the left and on the right have a paragraph for the instruction, just like

If the special symbols are hard to add, we can leave a text paragraph for future polish.

superstar54 · 2022-10-21T12:25:51Z

keep the matrix on the left and on the right have a paragraph for the instruction, just like

Will this make too much text on the GUI? I think a detailed explanation would be better put in the documentation.

unkcpz · 2022-10-21T15:13:30Z

Will this make too much text on the GUI? I think a detailed explanation would be better put in the documentation.

Emm... I can't agree with this, since the structure editor widget is default hidden and only shows when the user needs to use it. Meanwhile, we don't have documentation for QeApp, the application itself needs to be explainable.

After discussing in person, I now understand what the first matrix was used for, thanks!!
I am a VESTA user, so I also suggest to using the similar format for it which is the a,b,c, alpha, beta, gamma for cell setting.
Like:

yakutovicha · 2022-10-21T15:15:33Z

@cpignedoli, could you please also have a look/test this PR? Just to make sure that it is implemented according to the user's needs.

cpignedoli · 2022-10-22T08:05:05Z

Great idea @superstar54 , I had requests to implement a cell editor to create surfaces but still had no time to go in this direction.
Happy to see that things started.

I have a few comments from my experience with the simulation of surfaces:
typically I start from a problem defined, for example, as "simulate a molecule on a stepped ( e.g. Au(11 11 12) ) gold surface

Or simulate the S1(110) surface.

What I do is
1)I start from a bulk and scale the bulk to the desired lattice parameter (will I use the exp lattice parameter, or the one compatible with my DFT settings, or, am I studying a stressed crystal,..)

2)I enter the cleavage plane in the editor (e.g. 1, 1 ,1 or 11, 11, 12) and select a thickness for the slab (this is definitely not intuitive with high indexes, you need the graphical support..), and the amount of vacuum I want to add (can also be 0 if I want to simulate a bulk in "a surface cell" (e.g. to compute projected bulk band structures) and I click the "cleavage button"

I adjust the thickness to the desired one

4)I "orient to standard" so that a1 coincides with x, a2 is in the xy plane (or I select a different standard if needed)

At this point in many cases (e.g. (111) of a cubic crystal that would produce a hexagonal cell) I redefine the lattice as a linear combination of the primitive 2D vectors to obtain for example a rectangular cell (e.g. I typically work not with the hexagonal Au(111) but with (a1 +a2 , a2-a1) that is rectangular

6)I reorient to standard

I replicate as desired

All of this without deriving myself the transformation matrices because for sure I would do hundreds of mistakes.

I hope my comments can be useful. We can also have a zoom call to see more examples

superstar54 · 2022-10-31T10:27:05Z

@cpignedoli Thanks for the details. Very helpful.

1)I start from a bulk and scale the bulk to the desired lattice parameter (will I use the exp lattice parameter, or the one compatible with my DFT settings, or, am I studying a stressed crystal,..)

This PR tries to implement this (editing cell).

For the rest of the steps (building a surface slab), I think it's better to open a new PR.

I suggest using ase.build.surface to handle surface index, thickness, rectangular and so on.

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

AndresOrtegaGuerrero · 2023-03-31T09:55:16Z

@yakutovicha Should we merge this PR , so the QE app can have the supercell and cell parameters editor?

unkcpz · 2023-08-03T09:48:38Z

Can anyone remind me what blocks this PR? Please add a comment.

unkcpz · 2023-10-05T17:11:45Z

Hi @yakutovicha @cpignedoli, any update on this PR? Can we get only the supercell functionality merged and if there are more you want to add to this editor, we leave it to the future PRs?

yakutovicha · 2023-10-12T11:36:03Z

Hi @yakutovicha @cpignedoli, any update on this PR? Can we get only the supercell functionality merged and if there are more you want to add to this editor, we leave it to the future PRs?

We briefly discussed this with PR @cpignedoli. We will give it a look this afternoon.

cpignedoli

I think the editor is great but difficult to use for a user not expert. It should be merged but we should afterwards add few features:

axes.. this is a viewer problem but becomes crucial here
HTML help text with e.g. an example on how you get a 111 surface from a gold conventional cell (or, how do I get Au(788) )
button to reset the matrix to 1 1 1
the matrix should be probably reset when we import a new structure (but teh button above is enough)

There is one issue that in my opinion should be implemented before merging:
when changing the cell vectors I need the checkbox "scale coordiantes"

checkbox marked: I use this e.g. to change lattice parameter of my bulk (I uploaded a structure that has exp lattice parameter I want to scale it to PBE lattice parameter)
checkbox not marked: I want to set to 40 the z axis to add vacuum, the atomic positions should not be affected

unkcpz · 2023-10-16T09:09:25Z

thanks @cpignedoli.

@superstar54 please rebase this PR with the master branch and see if you can address the points mentioned by @cpignedoli. Thanks. As we discussed, this feature is better included in the next release.

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

codecov · 2023-10-18T09:51:35Z

Codecov Report

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

Comparison is base (fc0e7e1) 85.76% compared to head (a1368c3) 85.82%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #372      +/-   ##
==========================================
+ Coverage   85.76%   85.82%   +0.06%     
==========================================
  Files          27       27              
  Lines        4553     4608      +55     
==========================================
+ Hits         3905     3955      +50     
- Misses        648      653       +5

Flag	Coverage Δ
python-3.10	`85.82% <90.90%> (+0.06%)`	⬆️
python-3.8	`85.86% <90.90%> (+0.06%)`	⬆️
python-3.9	`85.86% <90.90%> (+0.06%)`	⬆️

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

Files	Coverage Δ
tests/test_structures.py	`100.00% <100.00%> (ø)`
aiidalab_widgets_base/structures.py	`84.06% <89.36%> (+0.37%)`	⬆️

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

superstar54 · 2023-10-18T09:53:07Z

Thanks @cpignedoli, for the suggestions. I have implemented:

button to reset the matrix to 1 1 1
the matrix should be probably reset when we import a new structure
checkbox to scale atoms positions
unit test

For the other two comments:

axes.. this is a viewer problem but becomes crucial here

I agree. We should address this in another PR.

HTML help text with e.g. an example on how you get a 111 surface from a gold conventional cell (or, how do I get Au(788) )

Note, that this PR is only used to edit the cell parameters, or make supercell, not to cut the surface.

superstar54 · 2023-11-01T14:47:01Z

Hi @yakutovicha would you like to review this, or should I proceed with merging it?

yakutovicha · 2023-11-02T13:24:32Z

Hi @yakutovicha would you like to review this, or should I proceed with merging it?

Let me give this PR a look today, just to be sure I didn't miss anything.

It will be quick, I hope 😄

yakutovicha · 2023-11-02T20:48:25Z

I agree. We should address this in another PR.

I made an issue for this #532

yakutovicha · 2023-11-02T20:50:26Z

In that

Note, that this PR is only used to edit the cell parameters, or make supercell, not to cut the surface.

I guess in this case we should modify the comment at the top, which says

Add the following features (#367, #365)

to prevent an accidental issue close

yakutovicha

thanks @superstar54.

I requested a bunch of trivial changes, once done - good to go from my side 👍

aiidalab_widgets_base/structures.py

yakutovicha · 2023-11-02T21:13:09Z

aiidalab_widgets_base/structures.py

+        # reset transformation matrix
+        self.reset_cell_transformation_matrix()


Suggested change

# reset transformation matrix

self.reset_cell_transformation_matrix()

I suggest not resetting the matrix automatically. The user will lose the information they entered.

I think it is safer to do that only when pressing the "Reset matrix" button.

I changed as you suggested.

aiidalab_widgets_base/structures.py

superstar54 · 2023-11-06T15:58:53Z

Hi @yakutovicha , thanks for your review. I've incorporated the changes based on your suggestions.

yakutovicha

LGTM, thanks @superstar54!

cell editor and transformation

8ead48d

This was linked to issues Oct 19, 2022

Cell Editor: enable surface cleavage. #367

Open

support building supercell. #365

Closed

superstar54 requested review from unkcpz and yakutovicha October 19, 2022 05:59

superstar54 changed the title ~~cell editor and transformation~~ Enhance cell editor Oct 19, 2022

superstar54 and others added 4 commits November 22, 2022 13:18

Merge branch 'master' into cell_transformation

f3a4d4d

change cell_vectors to cell_parameters

6248ff3

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

64c89c5

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

Merge branch 'master' into cell_transformation

8d29d3f

unkcpz added this to the v2.1.0 milestone Oct 4, 2023

cpignedoli reviewed Oct 12, 2023

View reviewed changes

superstar54 and others added 3 commits October 18, 2023 11:01

Merge branch 'master' into cell_transformation

4f95704

add scale_atoms, and reset transformatioin matrix

a1c83f7

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

8a683a6

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

superstar54 requested a review from cpignedoli October 18, 2023 09:53

cpignedoli previously approved these changes Oct 18, 2023

View reviewed changes

Merge branch 'master' into cell_transformation

8eee469

yakutovicha mentioned this pull request Nov 2, 2023

Structure viewer: add a button to display axes. #532

Open

yakutovicha requested changes Nov 2, 2023

View reviewed changes

yakutovicha and others added 2 commits November 6, 2023 16:07

Merge branch 'master' into cell_transformation

6d0a04c

add comment and make function private

dd90b7d

superstar54 dismissed cpignedoli’s stale review via dd90b7d November 6, 2023 15:48

update test

a1368c3

superstar54 requested a review from yakutovicha November 6, 2023 15:57

yakutovicha approved these changes Nov 6, 2023

View reviewed changes

superstar54 merged commit 1a28fe9 into master Nov 7, 2023
15 checks passed

superstar54 deleted the cell_transformation branch November 7, 2023 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance cell editor #372

Enhance cell editor #372

superstar54 commented Oct 19, 2022 •

edited by yakutovicha

Loading

unkcpz commented Oct 21, 2022

superstar54 commented Oct 21, 2022

unkcpz commented Oct 21, 2022

superstar54 commented Oct 21, 2022

unkcpz commented Oct 21, 2022

yakutovicha commented Oct 21, 2022 •

edited

Loading

cpignedoli commented Oct 22, 2022

superstar54 commented Oct 31, 2022

AndresOrtegaGuerrero commented Mar 31, 2023

unkcpz commented Aug 3, 2023

unkcpz commented Oct 5, 2023

yakutovicha commented Oct 12, 2023

cpignedoli left a comment

unkcpz commented Oct 16, 2023

codecov bot commented Oct 18, 2023 •

edited

Loading

superstar54 commented Oct 18, 2023

superstar54 commented Nov 1, 2023 •

edited

Loading

yakutovicha commented Nov 2, 2023 •

edited

Loading

yakutovicha commented Nov 2, 2023

yakutovicha commented Nov 2, 2023

yakutovicha left a comment

yakutovicha Nov 2, 2023 •

edited

Loading

superstar54 Nov 6, 2023

superstar54 commented Nov 6, 2023

yakutovicha left a comment

		# reset transformation matrix
		self.reset_cell_transformation_matrix()

Enhance cell editor #372

Enhance cell editor #372

Conversation

superstar54 commented Oct 19, 2022 • edited by yakutovicha Loading

unkcpz commented Oct 21, 2022

superstar54 commented Oct 21, 2022

unkcpz commented Oct 21, 2022

superstar54 commented Oct 21, 2022

unkcpz commented Oct 21, 2022

yakutovicha commented Oct 21, 2022 • edited Loading

cpignedoli commented Oct 22, 2022

superstar54 commented Oct 31, 2022

AndresOrtegaGuerrero commented Mar 31, 2023

unkcpz commented Aug 3, 2023

unkcpz commented Oct 5, 2023

yakutovicha commented Oct 12, 2023

cpignedoli left a comment

Choose a reason for hiding this comment

unkcpz commented Oct 16, 2023

codecov bot commented Oct 18, 2023 • edited Loading

Codecov Report

superstar54 commented Oct 18, 2023

superstar54 commented Nov 1, 2023 • edited Loading

yakutovicha commented Nov 2, 2023 • edited Loading

yakutovicha commented Nov 2, 2023

yakutovicha commented Nov 2, 2023

yakutovicha left a comment

Choose a reason for hiding this comment

yakutovicha Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

superstar54 Nov 6, 2023

Choose a reason for hiding this comment

superstar54 commented Nov 6, 2023

yakutovicha left a comment

Choose a reason for hiding this comment

superstar54 commented Oct 19, 2022 •

edited by yakutovicha

Loading

yakutovicha commented Oct 21, 2022 •

edited

Loading

codecov bot commented Oct 18, 2023 •

edited

Loading

superstar54 commented Nov 1, 2023 •

edited

Loading

yakutovicha commented Nov 2, 2023 •

edited

Loading

yakutovicha Nov 2, 2023 •

edited

Loading