Skip to content

Fix `Prism` to reshape cell correctly (and not to reshape cell by default (so just rotate))

Yuji Ikeda requested to merge yuzie007/ase:fix-prism into master

Short summary

This MR modifies Prism not to reshape the cell by default, together with fixing / refactoring.

Closes #1006 (closed) #1261 (closed)

Motivation

Technically LAMMPS CAN calculate a system with a "largely" tilted cell. We can specify

box tilt large

Then LAMMPS just gives a warning but continues the calculation (see also my note below).

The present implementation of Prism also gives inconsistent tilted cells with the official LAMMPS documentation (as well as pymatgen, as reported in #1261 (closed)). It also gives unexpected inconsistency between the tilted cell and the atomic positions (#1006 (closed)).

Considering this and seeing troubles in #1006 (closed) and #1261 (closed), it would be good to fix / refactor the Prism object, and it is also nice not to tilt the cell in Prism, at least by default, to avoid confusion. The present MR addresses these.

Commits

The beginning of the commits focused on refactoring and adding tests for Prism.

The reshaping of the cell can still be done in Prism via the new reduced option. This option can be accessed from write_lammps_data via the reduce_cell option. The LAMMPS calculators also uses write_lammps_data and has the same reduce_cell option now.

Now box tilt large is always set in LAMMPS. As detailed in my note below, this does not sacrifice the efficiency even for systems with small tilts and therefore worth having always. (Indeed since version 22Dec2022 LAMMPS even ignores this option and always accept tilted cells by default. This implies that the LAMMPS community also thought this is rational.)

I also fixed Prism for reduced=True cases. Before Prism did not do correct cell reduction systems very large tilts (e.g. cell=((3.0, 0.0, 0.0), (5.0, 3.0, 0.0), (0.0, 0.0, 3.0)), i.e., the x coordinates of the b axis is beyond that of a axis). Now this is fixed (tests are also in the present MR.)

Notes

LAMMPS box command

Before LAMMPS by default ... Since version 22Dec2022 LAMMPS even removed the box command and accepts largely tilted cells by default.

Due to the deprecation, the box command is not anymore documented officially, but we can still find its details in e.g. https://www.afs.enea.it/software/lammps/doc19/html/box.html.

Note that box tilt large does not affect the efficiency for systems with small tilts at all. As far as I surveyed, this command is used only to decide if it raises an error or a warning (see links below) and does nothing furthermore.

Extremely tilted cells?

Someone might worry that untilted cells in Prism might cause potential issues in LAMMPS. I would say, if the tilts are modest (i.e., the tilt factors slightly more than half the distance of the parallel box length), LAMMPS should still be able to run safely for such edge cases (e.g. hcp structures).

For an extremely tilted cell, this is dangerous not only for LAMMPS but in general for any other classical-potential calculations (as the distances between atoms may be wrongly evaluated etc.) as well as DFT. I would say it is therefore on the user side to take the responsibility to address such an extremely titled cell, i.e., we should do e.g. the Niggli reduction before the calculation by ourselves. (Nevertheless, Prism can still reshape the cell by the new reduced option.)

LAMMPSlib

LAMMPSlib is not modified in the MR, simply because it does not use Prism. If I understand correctly, this means that LAMMPSlib does not reshape the cell and just rotates it. This is actually the consistent behavior with the revised LAMMPSRun in this MR.

calc_box_parameters etc.

Actually the implementation calc_box_parameters and calc_lammps_tilt is essentially the same as lammpslib.convert_cell and thus can / should be unified. In this MR I haven't done this to avoid further complexity, but after the MR is accepted I will work on.

wrap in write_lammps_data

With better understanding of Prism now, I think I can also add the wrap option for write_lammps_data, which will be done in a separate MR.

Merge request reports