Generate automatic lattice vectors in loadGeometry #216

mondracek · 2023-10-24T10:22:27Z

Fixes #52
Supersedes #213 and #197

…ini nor the input file specify them Moreover - remove premature call to autoGridN in loadParams - enable reading ASE-style lattice vector from XYZ in ppafm_gui.py - handle fzs/Fout deletion in plot_results.py correctly depending on tip tilt - correct typo Lenard-Jones -> Lennard-Jones - add 'self' argument to prepareFEConv() in relax.py to console precommit checks - resolve conflicts in formatting resulting from limits on line length Set grid in computeELFF_pointCharge before parsing atoms Correct swapped PBC / not PBC condition in loadGeometry Correct format of atomic data returned by loadGeometry - accept the solution from branch load-geometry-fix by Niko Oinonen - read 'nDim' from 'gridN' for POSCAR and geometry.in inputs Repair a comment line in io.py broken by automatic reformatting Repair an indentation error in ppafm/HighLevel.py and typo in ppafm/cpp/Force.h

mondracek · 2023-10-24T12:59:11Z

The formatting changes were removed with the help of @yakutovicha and the other commits squashed into one.

ppafm/io.py

mondracek · 2023-10-25T09:58:41Z

@NikoOinonen , I implemented the amendments you proposed (not pushed yet). Restricting the automatic cell to the scan region (plus some padding) works okay for non-periodic structures. It leads to wrong results for PBC True in params.ini. That would not be a problem since the automatically generated cell is intended for clusters and isolated molecules while for periodic structures, the lattice vectors should be given explicitly. However, it may become a problem because params["PBC"]=True is our default now. I have already proposed to make PBC False the default in the discussion under #52 but forgot about it later.

What I'd proposed now: Make params["PBC"]=None by default and unless PBC True or PBC False is specified explicitly in params.ini, make it True when reading the input from *.xsf or from geometry.in without lattice_vectors, and make it False when reading from the *.xyz file or a geometry.in file without lattice_vectors. I am not sure about *.cube files, whether they should change params["PBC"]=None to params["PBC"]=True or to params["PBC"]=False. Probably to False as the CUBE format is originally intended for non-periodic systems although FHI-AIMS often uses it for periodic ones too. As for the XSF format, it could actually describe a non-periodic system too, but we are currently not making the distinction. Should I implement this default handling of PBC in this PR?

NikoOinonen · 2023-10-25T12:01:31Z

What I'd proposed now: Make params["PBC"]=None by default and unless PBC True or PBC False is specified explicitly in params.ini

Yes, it makes sense to have some default behaviour when not otherwise specified.

make it True when reading the input from *.xsf or from geometry.in with lattice_vectors, and make it False when reading from the *.xyz file or a geometry.in file without lattice_vectors. I am not sure about *.cube files, whether they should change params["PBC"]=None to params["PBC"]=True or to params["PBC"]=False.

Would it not be fine to just set it True whenever the lattice vectors are defined in the input regardless of the format (when not explicitly set as False)? Especially for cube files (and for xsf), the convolution of the Hartree potential is always periodic just because of how the operation is defined, so we end up using periodic boundary conditions anyways.

Should I implement this default handling of PBC in this PR?

Yes, I think it makes sense, since it affects the behaviour here.

mondracek · 2023-10-25T15:23:54Z

Right, I was about to do it that way after all: Set params["PBC"] in loadGeometry depending on whether the lvec is None or something else. However, I realized we must not initialize params["PBC"] as None in common.py as I tried to do. The type of each parameter is determined by its initial value and when it is unknown, reading the parameter from params.ini does not work. So I will have to leave the default value of params["PBC"] to be True. But then, I must not force it to be False even if e.g. the input is a *xyz file without lattice vectors because the lattice vectors can still be specified as the gridA, gridB, gridC parameters in params.ini and the user can mean these as true lattice vectors rather than just dimensions of a non-periodic grid.
So in the end, the only thing I can do with params["PBC"] in response to the geometry read from the input is to put params["PBC"]=False exactly when the automatic grid is being generated and leave it as it was (set by default or by the user in params.ini) otherwise.

- Sets PBC to False whenever automatic grid needs to be created - Remove the back-and-forth transformation between pivot-particle and probe-particle coordinates

mondracek · 2023-10-26T09:00:26Z

Pushed a new version that reflects the recent discussion.

ppafm/io.py

NikoOinonen · 2023-10-26T09:32:44Z

ppafm/io.py

+        if np.allclose(lvec[i + 1, :], 0):
+            params["PBC"] = False
+            lvec[i + 1, i] = probeMax[i] + pad


It seems to me like we are not taking into account the start of the scan region here. I think we would need to adjust the origin of the lvec to be at something like probeMin - pad, and then the length of the lattice vectors would be something like probeMax - probeMin + 2 * pad? Notice that currently probeMin is not used.

Exactly right. The thing is, the current version of the code (at least the CPU part, I do not know about the OCL) does not work with non-zero origin in non-periodic systems. I should and I will solve this, but as a separate issue. In fact, I have added a line #lvec[i + 1, i] = probe_max[i] - probe_min[i] + 2*pad to io.py, which is commented-out for now but which I intend to use instead of the contemporary lvec[i + 1, i] = probe_max[i] + pad as soon as I solve the issue of an arbitrary origin.

Ok, I didn't know that. It should work with any origin in the OCL code. I guess this by extension means that negative scan window coordinates don't work when PBC = False. Is there an issue for this? If not, then better make one so that it does not slip by.

Exactly, last time I tried I got a blank area with zero forces for the part of scan region in which any of the three coordinates negative. There is no issue about it and I am going to make one. The original plan was to solve it as a by-product of #217 (may seem to be unrelated but as FHI-AIMS tends to be creative about the grid origin, it would have to be done to address that issue too). However, as I am being rather discouraged to implement #217, I will start a separate issue.

mondracek · 2023-10-26T09:47:17Z

ppafm/io.py

@@ -451,7 +451,8 @@ def loadGeometry(fname=None,format=None,params=None):
        # The automatic generated lattice vector should enclose the whole area filled with atoms as well as the whole scanning area, plus the default padding.
        if np.allclose(lvec[i + 1, :], 0):
            params["PBC"] = False
-            lvec[i + 1, i] = probeMax[i] + pad
+            #lvec[i + 1, i] = probe_max[i] - probe_min[i] + 2*pad


This comment line will be activated once we deal with a potentially non-zero origin properly.

NikoOinonen

Ok, looks good now. Thanks Martin!

mondracek requested a review from NikoOinonen October 24, 2023 12:57

NikoOinonen reviewed Oct 24, 2023

View reviewed changes

ppafm/io.py Outdated Show resolved Hide resolved

NikoOinonen reviewed Oct 24, 2023

View reviewed changes

ppafm/io.py Outdated Show resolved Hide resolved

Limit the automatic grid to non-periodic cases

2f8a3e4

- Sets PBC to False whenever automatic grid needs to be created - Remove the back-and-forth transformation between pivot-particle and probe-particle coordinates

NikoOinonen requested changes Oct 26, 2023

View reviewed changes

Naming convention: probeMin/probeMax -> probe_min/probe_max

732ee23

mondracek commented Oct 26, 2023

View reviewed changes

Repair a comment about the automatic grid in io.py

ff8492d

NikoOinonen approved these changes Oct 26, 2023

View reviewed changes

mondracek merged commit 537851d into main Oct 26, 2023

mondracek deleted the automatic-lvs-next branch October 26, 2023 11:57

This was referenced Nov 1, 2023

Atoms with negative coordinates in non-periodic systems invisible (in the CPU version) #223

Closed

Suggest cleanup of geometry handling in the CPU version #198

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate automatic lattice vectors in loadGeometry #216

Generate automatic lattice vectors in loadGeometry #216

mondracek commented Oct 24, 2023

mondracek commented Oct 24, 2023

mondracek commented Oct 25, 2023 •

edited

Loading

NikoOinonen commented Oct 25, 2023

mondracek commented Oct 25, 2023

mondracek commented Oct 26, 2023

NikoOinonen Oct 26, 2023

mondracek Oct 26, 2023

NikoOinonen Oct 26, 2023

mondracek Oct 26, 2023

mondracek Oct 26, 2023 •

edited

Loading

NikoOinonen left a comment

Generate automatic lattice vectors in loadGeometry #216

Generate automatic lattice vectors in loadGeometry #216

Conversation

mondracek commented Oct 24, 2023

mondracek commented Oct 24, 2023

mondracek commented Oct 25, 2023 • edited Loading

NikoOinonen commented Oct 25, 2023

mondracek commented Oct 25, 2023

mondracek commented Oct 26, 2023

NikoOinonen Oct 26, 2023

Choose a reason for hiding this comment

mondracek Oct 26, 2023

Choose a reason for hiding this comment

NikoOinonen Oct 26, 2023

Choose a reason for hiding this comment

mondracek Oct 26, 2023

Choose a reason for hiding this comment

mondracek Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

NikoOinonen left a comment

Choose a reason for hiding this comment

mondracek commented Oct 25, 2023 •

edited

Loading

mondracek Oct 26, 2023 •

edited

Loading