-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for having non-displaced atoms in Phonopy routines #2110
base: main
Are you sure you want to change the base?
Changes from all commits
d2aa51a
24608c9
d6deffc
1f31546
a7b3a0c
38a4230
7430b8a
6939b90
54c4e5f
b7789d2
5536691
93a66a6
75b8f9f
56fc1ac
b98984e
15f4469
e715946
f39bf76
2225c9d
0d06540
8817af3
923ca89
2f3e943
1d710db
7329f9b
e102d40
98d0274
4f1027e
0dec274
5f99ce4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ | |
|
||
if has_phonopy: | ||
from phonopy import Phonopy | ||
from phonopy.structure.cells import get_supercell | ||
|
||
if TYPE_CHECKING: | ||
from ase.atoms import Atoms | ||
|
@@ -25,7 +26,7 @@ | |
@requires(has_phonopy, "Phonopy not installed.") | ||
def get_phonopy( | ||
atoms: Atoms, | ||
min_lengths: float | tuple[float, float, float] | None = None, | ||
min_lengths: float | tuple[float, float, float] | None = 20.0, | ||
supercell_matrix: ( | ||
tuple[tuple[int, int, int], tuple[int, int, int], tuple[int, int, int]] | None | ||
) = None, | ||
|
@@ -91,3 +92,32 @@ def phonopy_atoms_to_ase_atoms(phonpy_atoms: PhonopyAtoms) -> Atoms: | |
""" | ||
pmg_structure = get_pmg_structure(phonpy_atoms) | ||
return pmg_structure.to_ase_atoms() | ||
|
||
|
||
def get_atoms_supercell_by_phonopy( | ||
atoms: Atoms, | ||
supercell_matrix: tuple[ | ||
tuple[int, int, int], tuple[int, int, int], tuple[int, int, int] | ||
], | ||
) -> Atoms: | ||
""" | ||
Get the supercell of an ASE atoms object using a supercell matrix. | ||
|
||
Parameters | ||
---------- | ||
atoms | ||
ASE atoms object. | ||
supercell_matrix | ||
The supercell matrix to use. If specified, it will override any | ||
value specified by `min_lengths`. | ||
Comment on lines
+111
to
+112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no |
||
Returns | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing a line break before |
||
------- | ||
Atoms | ||
ASE atoms object of the supercell. | ||
""" | ||
|
||
return phonopy_atoms_to_ase_atoms( | ||
get_supercell( | ||
get_phonopy_structure(Structure.from_ase_atoms(atoms)), supercell_matrix | ||
) | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,10 +5,15 @@ | |
from importlib.util import find_spec | ||
from typing import TYPE_CHECKING | ||
|
||
from ase.atoms import Atoms | ||
from monty.dev import requires | ||
|
||
from quacc import job, subflow | ||
from quacc.atoms.phonons import get_phonopy, phonopy_atoms_to_ase_atoms | ||
from quacc.atoms.phonons import ( | ||
get_atoms_supercell_by_phonopy, | ||
get_phonopy, | ||
phonopy_atoms_to_ase_atoms, | ||
) | ||
from quacc.runners.phonons import PhonopyRunner | ||
from quacc.schemas.phonons import summarize_phonopy | ||
|
||
|
@@ -18,21 +23,17 @@ | |
if TYPE_CHECKING: | ||
from typing import Any | ||
|
||
from ase.atoms import Atoms | ||
|
||
from quacc import Job | ||
from quacc.schemas._aliases.phonons import PhononSchema | ||
|
||
if has_phonopy: | ||
from phonopy import Phonopy | ||
|
||
|
||
@subflow | ||
@requires(has_phonopy, "Phonopy must be installed. Run `pip install quacc[phonons]`") | ||
@requires(has_seekpath, "Seekpath must be installed. Run `pip install quacc[phonons]`") | ||
def phonon_subflow( | ||
atoms: Atoms, | ||
displaced_atoms: Atoms, | ||
force_job: Job, | ||
non_displaced_atoms: Atoms | None = None, | ||
symprec: float = 1e-4, | ||
min_lengths: float | tuple[float, float, float] | None = 20.0, | ||
supercell_matrix: ( | ||
|
@@ -48,12 +49,22 @@ def phonon_subflow( | |
""" | ||
Calculate phonon properties. | ||
|
||
In Quacc the ASE constraints can be used to fix atoms. These atoms will | ||
not be displaced during the phonon calculation. This will greatly reduce | ||
the computational cost of the calculation. However, this is an important | ||
approximation and should be used with caution. | ||
Comment on lines
+52
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is relevant anymore. |
||
|
||
Parameters | ||
---------- | ||
atoms | ||
displaced_atoms | ||
Atoms object with calculator attached. | ||
force_job | ||
The static job to calculate the forces. | ||
non_displaced_atoms | ||
Additional atoms to add to the supercells i.e. fixed atoms. | ||
These atoms will not be displaced during the phonon calculation. | ||
Useful for adsorbates on surfaces with weak coupling etc. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was my point of confusion. I had thought you were implying that additional atoms would be for the adsorbates (I now see that's not the case). In reality, it would be for the surface of course. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I completely see where this confusion can get from, this is why I am not entirely sure about the current name. Although this forces the users to stop and think before using this important approximation EDIT: yes, the doc does not help as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One downside of I think either could be okay, personally. |
||
Important approximation, use with caution. | ||
symprec | ||
Precision for symmetry detection. | ||
min_lengths | ||
|
@@ -80,6 +91,27 @@ def phonon_subflow( | |
Dictionary of results from [quacc.schemas.phonons.summarize_phonopy][] | ||
""" | ||
|
||
non_displaced_atoms = non_displaced_atoms or Atoms() | ||
|
||
phonopy = get_phonopy( | ||
displaced_atoms, | ||
min_lengths=min_lengths, | ||
supercell_matrix=supercell_matrix, | ||
symprec=symprec, | ||
displacement=displacement, | ||
phonopy_kwargs=phonopy_kwargs, | ||
) | ||
|
||
if non_displaced_atoms: | ||
non_displaced_atoms = get_atoms_supercell_by_phonopy( | ||
non_displaced_atoms, phonopy.supercell_matrix | ||
) | ||
|
||
supercells = [ | ||
phonopy_atoms_to_ase_atoms(s) + non_displaced_atoms | ||
for s in phonopy.supercells_with_displacements | ||
] | ||
|
||
@subflow | ||
def _get_forces_subflow(supercells: list[Atoms]) -> list[dict]: | ||
return [ | ||
|
@@ -89,17 +121,25 @@ def _get_forces_subflow(supercells: list[Atoms]) -> list[dict]: | |
@job | ||
def _thermo_job( | ||
atoms: Atoms, | ||
phonopy: Phonopy, | ||
phonopy, | ||
force_job_results: list[dict], | ||
t_step: float, | ||
t_min: float, | ||
t_max: float, | ||
additional_fields: dict[str, Any] | None, | ||
t_step, | ||
t_min, | ||
t_max, | ||
additional_fields, | ||
Comment on lines
+124
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you just removed the type hints for brevity's sake since they are basically implied elsewhere? I was also thinking about this since it is an internal function. Although, if we at some point enabled static type checking, having the type hints would ensure there isn't an error. I doubt we will be able to get to the point where we enable static type checking though... certainly not anytime soon. |
||
) -> PhononSchema: | ||
parameters = force_job_results[-1].get("parameters") | ||
forces = [output["results"]["forces"] for output in force_job_results] | ||
forces = [ | ||
output["results"]["forces"][: len(phonopy.supercell)] | ||
for output in force_job_results | ||
] | ||
phonopy_results = PhonopyRunner().run_phonopy( | ||
phonopy, forces, t_step=t_step, t_min=t_min, t_max=t_max | ||
phonopy, | ||
forces, | ||
symmetrize=bool(non_displaced_atoms), | ||
t_step=t_step, | ||
t_min=t_min, | ||
t_max=t_max, | ||
) | ||
|
||
return summarize_phonopy( | ||
|
@@ -110,18 +150,13 @@ def _thermo_job( | |
additional_fields=additional_fields, | ||
) | ||
|
||
phonopy = get_phonopy( | ||
atoms, | ||
min_lengths=min_lengths, | ||
supercell_matrix=supercell_matrix, | ||
symprec=symprec, | ||
displacement=displacement, | ||
phonopy_kwargs=phonopy_kwargs, | ||
) | ||
supercells = [ | ||
phonopy_atoms_to_ase_atoms(s) for s in phonopy.supercells_with_displacements | ||
] | ||
force_job_results = _get_forces_subflow(supercells) | ||
return _thermo_job( | ||
atoms, phonopy, force_job_results, t_step, t_min, t_max, additional_fields | ||
displaced_atoms, | ||
phonopy, | ||
force_job_results, | ||
t_step, | ||
t_min, | ||
t_max, | ||
additional_fields, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,13 +19,14 @@ | |
|
||
@flow | ||
def phonon_flow( | ||
atoms: Atoms, | ||
displaced_atoms: Atoms, | ||
symprec: float = 1e-4, | ||
min_lengths: float | tuple[float, float, float] | None = 20.0, | ||
supercell_matrix: ( | ||
tuple[tuple[int, int, int], tuple[int, int, int], tuple[int, int, int]] | None | ||
) = None, | ||
displacement: float = 0.01, | ||
non_displaced_atoms: Atoms | None = None, | ||
t_step: float = 10, | ||
t_min: float = 0, | ||
t_max: float = 1000, | ||
|
@@ -50,7 +51,7 @@ def phonon_flow( | |
|
||
Parameters | ||
---------- | ||
atoms | ||
displaced_atoms | ||
Atoms object | ||
symprec | ||
Precision for symmetry detection. | ||
|
@@ -61,6 +62,11 @@ def phonon_flow( | |
value specified by `min_lengths`. | ||
displacement | ||
Atomic displacement (A). | ||
non_displaced_atoms | ||
Additional atoms to add to the supercells i.e. fixed atoms. | ||
These atoms will not be displaced during the phonon calculation. | ||
Useful for adsorbates on surfaces with weak coupling etc. | ||
Important approximation, use with caution. | ||
t_step | ||
Temperature step (K). | ||
t_min | ||
|
@@ -90,15 +96,16 @@ def phonon_flow( | |
param_swaps=job_params, | ||
decorators=job_decorators, | ||
) | ||
if run_relax: | ||
atoms = relax_job_(atoms)["atoms"] | ||
if run_relax and not non_displaced_atoms: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a little bit problematic, another option is to send both displaced and non_displaced and optimise them and separate them again... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. I mean, I guess we can ask if we really even need a relaxation step beforehand? Presumably one could just call If there's a desire to keep this though, then yeah I think the only route would be to relax |
||
displaced_atoms = relax_job_(displaced_atoms)["atoms"] | ||
|
||
return phonon_subflow( | ||
atoms, | ||
displaced_atoms, | ||
static_job_, | ||
symprec=symprec, | ||
min_lengths=min_lengths, | ||
supercell_matrix=supercell_matrix, | ||
non_displaced_atoms=non_displaced_atoms, | ||
displacement=displacement, | ||
t_step=t_step, | ||
t_min=t_min, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ | |
pytest.importorskip("phonopy") | ||
pytest.importorskip("seekpath") | ||
|
||
from ase.build import bulk | ||
from ase.build import bulk, molecule | ||
|
||
from quacc.recipes.emt.phonons import phonon_flow | ||
|
||
|
@@ -70,4 +70,22 @@ def test_phonon_flow_v4(tmp_path, monkeypatch): | |
assert output["results"]["thermal_properties"]["temperatures"][-1] == 1000 | ||
assert output["results"]["force_constants"].shape == (8, 8, 3, 3) | ||
assert "mesh_properties" in output["results"] | ||
assert output["atoms"] != atoms | ||
assert output["atoms"] == atoms | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Based on the test failure in CI, it looks like the original There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this was related to #2230. |
||
|
||
|
||
def test_phonon_flow_fixed(tmp_path, monkeypatch): | ||
monkeypatch.chdir(tmp_path) | ||
atoms = molecule("H2", vacuum=20.0) | ||
output = phonon_flow(atoms, run_relax=True, min_lengths=5.0) | ||
|
||
atoms1 = molecule("H2", vacuum=20.0) | ||
atoms2 = molecule("H2", vacuum=20.0) | ||
|
||
atoms2.positions += [10, 10, 10] | ||
|
||
output_fixed = phonon_flow(atoms1, non_displaced_atoms=atoms2, min_lengths=5.0) | ||
|
||
# Should be very close but not exactly the same | ||
assert output["results"]["mesh_properties"]["frequencies"] == pytest.approx( | ||
output_fixed["results"]["mesh_properties"]["frequencies"], rel=0.0, abs=1e-5 | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal to have a simple unit test for this specific function.