Closed ndaelman-hu closed 8 months ago
@ladinesa Let me know if you prefer that Lauri reviews, since he originally wrote the Crystal parser.
Note, the conversion in to_system
seems somewhat superfluous, even after simplification.
Essentially, all coordinates are written as scaled wrt to the cell dimensions. In lower dimensionalities, the excluded axes are written in Cartesian coordinates. It's the scaled positions that are converted to Cartesian coordinates.
However, the first print-out of the geometric structure is fully Cartesian. You can find it under the line
AT.IRR. AT AT.N. X Y Z
I could just extract the starting structure as such, without the need for converting. From what I can tell, we still need the current approach for updates after ionic steps.
On the same thought, should we consider changing our communication of the PBC conditions? Technically, the code converts any non-periodic axis to one with 500 Angstrom. It is pretty verbose about this, printing it out in the lattice parameters. However, when a 0D system is run, it no longer prints out the lattice...
Looks good. thanks for the fix
Note, the conversion in to_system seems somewhat superfluous, even after simplification. Essentially, all coordinates are written as scaled wrt to the cell dimensions. In lower dimensionalities, the excluded axes are written in Cartesian coordinates. It's the scaled positions that are converted to Cartesian coordinates.
However, the first print-out of the geometric structure is fully Cartesian. You can find it under the line AT.IRR. AT AT.N. X Y Z I could just extract the starting structure as such, without the need for converting. From what I can tell, we still need the current approach for updates after ionic steps.
So, the problem lies with line 629:
# If any geometry edits (supercells, substitutions, dispplacements,
# deformations, nanotube construction, etc.) are done on top of the
# original system, they override the original system.
if system_edited is not None:
if system_edited["labels_positions_nanotube"] is not None: # pylint: disable=E1136
labels_positions = system_edited["labels_positions_nanotube"] # pylint: disable=E1136
else:
labels_positions = system_edited["labels_positions"] # pylint: disable=E1136
# TODO adjust re pattern for other formats e.g. with R(ANGS)
if labels_positions is not None:
atomic_numbers = labels_positions[:, 2] # pylint: disable=E1136
atom_labels = labels_positions[:, 3] # pylint: disable=E1136
atom_pos = labels_positions[:, 4:7] # pylint: disable=E1136
if system_edited["lattice_parameters"] is not None: # pylint: disable=E1136
lattice = system_edited["lattice_parameters"] # pylint: disable=E1136
Here, the format may be changed to include a mixture of scaled and Cartesian coordinates.
Looks good. thanks for the fix
What do I do with labels_positions_raw
?
Looks good. thanks for the fix
What do I do with
labels_positions_raw
?
Not sure what this is for, it is the starting structure? if I understand the issue right, probably you can set the argument to to_system for dimensionality to 0 for labels_positions_raw.
Looks good. thanks for the fix
What do I do with
labels_positions_raw
?Not sure what this is for, it is the starting structure? if I understand the issue right, probably you can set the argument to to_system for dimensionality to 0 for labels_positions_raw.
Indeed, labels_positions
and labels_positions_raw
both extract the starting structure.
labels_positions_raw
already has the right coordinates (all Cartesian) and can be stored without the need for to_system
.
lattice_positions
(and the subtypes) extract updates to the initial geometry (e.g. optimization, frequency, etc.).
Just as labels_positions
, they all follow a mixed coordinate system (scaled for axes with PBC, and Cartesian for the rest).
These do require to_system
.
So you see that we have 2 options on how to deal with this.
Looks good. thanks for the fix
What do I do with
labels_positions_raw
?Not sure what this is for, it is the starting structure? if I understand the issue right, probably you can set the argument to to_system for dimensionality to 0 for labels_positions_raw.
Indeed,
labels_positions
andlabels_positions_raw
both extract the starting structure.labels_positions_raw
already has the right coordinates (all Cartesian) and can be stored without the need forto_system
.
lattice_positions
(and the subtypes) extract updates to the initial geometry (e.g. optimization, frequency, etc.). Just aslabels_positions
, they all follow a mixed coordinate system (scaled for axes with PBC, and Cartesian for the rest). These do requireto_system
.So you see that we have 2 options on how to deal with this.
Just maybe use labels_positions. Leave the defintion for labels_positions_raw for now.
I fixed the lattice parameter assignment causing the bug in issue #170. It now uses the
dimensionality
to assigncart_pos
, rather than the system type. I also cleaned upto_system
, which assigned the Cartesian positions.I slightly extended the tests, but I can see to also include an actual sample of the original bug report. All examples from the bug report now pass, see figure.