Fix `Prism` to reshape cell correctly (and not to reshape cell by default (so just rotate))
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
box
command
LAMMPS Before LAMMPS by default ...
Since version 22Dec2022 LAMMPS even removed the box
command and accepts largely tilted cells by default.
- https://docs.lammps.org/Commands_removed.html#box-command
- https://github.com/lammps/lammps/pull/3527
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.
- https://github.com/lammps/lammps/blob/stable_23Jun2022_update4/src/domain.cpp#L217-L226
- https://github.com/lammps/lammps/blob/stable_23Jun2022_update4/src/domain.cpp#L1936-L1950
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.