170 broken parsing of 0d systems in crystal

[ndaelman-hu]

I fixed the lattice parameter assignment causing the bug in issue #170.
It now uses the dimensionality to assign cart_pos, rather than the system type.
I also cleaned up to_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.

[ndaelman-hu]

@ladinesa Let me know if you prefer that Lauri reviews, since he originally wrote the Crystal parser.

[ndaelman-hu]

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.

[ndaelman-hu]

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...

[ladinesa]

first time I have seen shape -1, good to know.

[ndaelman-hu]

I forgot that I left this capturing group in... It didn't work, but I fixed it now. Still, it isn't used anywhere atm.

[ladinesa]

Looks good. thanks for the fix

[ndaelman-hu]

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.

[ndaelman-hu]

Looks good. thanks for the fix

What do I do with labels_positions_raw?

[ladinesa]

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.

[ndaelman-hu]

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.

[ladinesa]

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.

Just maybe use labels_positions. Leave the defintion for labels_positions_raw for now.