Skip to content
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.

Sign up for GitHub

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

Fix ValueError when passing selective_dynamics to Poscar #2951

Merged
merged 12 commits into from
Apr 30, 2023
Merged

Fix ValueError when passing selective_dynamics to Poscar #2951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1b7d6b5
Fixed selective_dynamics error for poscar in vasp/inputs.py
Apr 24, 2023
a213a59
Merge branch 'materialsproject:master' into master
chiang-yuan Apr 27, 2023
4bc5cf3
add unit test
Apr 27, 2023
29a14ac
Revert "add unit test"
Apr 27, 2023
3477e2c
add unit test for selective dynamics in `io/vasp/tests`
Apr 27, 2023
9410370
pre-commit auto-fixes
pre-commit-ci[bot] Apr 27, 2023
84875dd
correct typing
Apr 27, 2023
4b6280c
Merge branch 'master' of github.com:chiang-yuan/pymatgen
Apr 27, 2023
b9724e2
pre-commit auto-fixes
pre-commit-ci[bot] Apr 27, 2023
0d561b5
clean up
janosh Apr 30, 2023
fc5a983
doc str to google format
janosh Apr 30, 2023
06fdab1
PoscarTest convert selective_dynamics to list[list[bool]] before compare
janosh Apr 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions pymatgen/core/tests/test_trajectory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,15 +164,9 @@ def test_site_properties(self):
species, coords, charge, spin = self._get_species_and_coords()

props = [
{
"test": [[True, True, True], [False, False, False]],
},
{
"test": [[False, False, False], [False, False, False]],
},
{
"test": [[True, True, True], [False, False, False]],
},
{"test": [[True, True, True], [False, False, False]]},
{"test": [[False, False, False], [False, False, False]]},
{"test": [[True, True, True], [False, False, False]]},
]
traj = Trajectory(
species=species,
Expand Down
59 changes: 35 additions & 24 deletions pymatgen/io/vasp/inputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,40 +94,51 @@ def __init__(
self,
structure: Structure,
comment: str | None = None,
selective_dynamics=None,
selective_dynamics: ArrayLike | None = None,
true_names: bool = True,
velocities: ArrayLike | None = None,
predictor_corrector: ArrayLike | None = None,
predictor_corrector_preamble: str | None = None,
sort_structure: bool = False,
):
"""
:param structure: Structure object.
:param comment: Optional comment line for POSCAR. Defaults to unit
cell formula of structure. Defaults to None.
:param selective_dynamics: bool values for selective dynamics,
where N is number of sites. Defaults to None.
:param true_names: Set to False if the names in the POSCAR are not
well-defined and ambiguous. This situation arises commonly in
vasp < 5 where the POSCAR sometimes does not contain element
symbols. Defaults to True.
:param velocities: Velocities for the POSCAR. Typically parsed
in MD runs or can be used to initialize velocities.
:param predictor_corrector: Predictor corrector for the POSCAR.
Typically parsed in MD runs.
:param predictor_corrector_preamble: Preamble to the predictor
corrector.
:param sort_structure: Whether to sort structure. Useful if species
are not grouped properly together.
Args:
structure (Structure): Structure object.
comment (str | None, optional): Optional comment line for POSCAR. Defaults to unit
cell formula of structure. Defaults to None.
selective_dynamics (ArrayLike | None, optional): Bool values for selective dynamics,
where N is the number of sites. Defaults to None.
true_names (bool, optional): Set to False if the names in the POSCAR are not
well-defined and ambiguous. This situation arises commonly in
VASP < 5 where the POSCAR sometimes does not contain element
symbols. Defaults to True.
velocities (ArrayLike | None, optional): Velocities for the POSCAR. Typically parsed
in MD runs or can be used to initialize velocities. Defaults to None.
predictor_corrector (ArrayLike | None, optional): Predictor corrector for the POSCAR.
Typically parsed in MD runs. Defaults to None.
predictor_corrector_preamble (str | None, optional): Preamble to the predictor
corrector. Defaults to None.
sort_structure (bool, optional): Whether to sort the structure. Useful if species
are not grouped properly together. Defaults to False.
"""
if structure.is_ordered:
site_properties = {}
if selective_dynamics:
site_properties["selective_dynamics"] = selective_dynamics
if velocities:
site_properties["velocities"] = velocities
if predictor_corrector:
site_properties["predictor_corrector"] = predictor_corrector

if selective_dynamics is not None:
selective_dynamics = np.array(selective_dynamics)
if not selective_dynamics.all():
site_properties["selective_dynamics"] = selective_dynamics
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chiang-yuan I simplified the code some. Was there a reason to convert np.array to list before assigning to site_props?

Copy link
Contributor Author

@chiang-yuan chiang-yuan Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janosh I converted np.array back to list because the other unit test in io/vasp/tests/test_inputs.py compare selective_dyanmics with list, which will yield the error in pytest like the one after your revision.

>           assert poscar.selective_dynamics == [[True, True, True], [False, False, False]]
E           ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

/home/runner/work/pymatgen/pymatgen/pymatgen/io/vasp/tests/test_inputs.py:87: ValueError

I bypass them by converting it back to list (which is what the original code intended to do) and adding another unit test comparing by list as well. Do you think it is better to revise the previous unit test and compare all of them in the format of nd.array like I suggested earlier?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make the test datatype agnostic so that it handles both arrays and lists. It's better for tests to care only about semantics and not equivalent ways of encoding the same meaning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Thanks @janosh !


if velocities is not None:
velocities = np.array(velocities)
if velocities.any():
site_properties["velocities"] = velocities

if predictor_corrector is not None:
predictor_corrector = np.array(predictor_corrector)
if predictor_corrector.any():
site_properties["predictor_corrector"] = predictor_corrector

structure = Structure.from_sites(structure)
self.structure = structure.copy(site_properties=site_properties)
if sort_structure:
Expand Down
32 changes: 31 additions & 1 deletion pymatgen/io/vasp/tests/test_inputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ def test_init(self):
0.750000 0.500000 0.750000 F F F O
"""
poscar = Poscar.from_string(poscar_string)
assert poscar.selective_dynamics == [[True, True, True], [False, False, False]]
selective_dynamics = [list(x) for x in poscar.selective_dynamics]

assert selective_dynamics == [[True, True, True], [False, False, False]]
self.selective_poscar = poscar

def test_from_file(self):
Expand Down Expand Up @@ -370,6 +372,34 @@ def test_write(self):
self.assertArrayAlmostEqual(poscar.structure.lattice.abc, p.structure.lattice.abc, 5)
tempfname.unlink()

def test_selective_dynamics(self):
filepath = PymatgenTest.TEST_FILES_DIR / "POSCAR.Fe3O4"
poscar = Poscar.from_file(filepath)
structure = poscar.structure

# Fix bottom half
fixed_indices = structure.frac_coords[:, 2] >= 0.5

poscar = Poscar(structure, selective_dynamics=np.tile(fixed_indices.reshape(-1, 1), [1, 3]))
selective_dynamics = [list(x) for x in poscar.selective_dynamics]

assert selective_dynamics == [
[True, True, True],
[False, False, False],
[False, False, False],
[True, True, True],
[False, False, False],
[True, True, True],
[True, True, True],
[False, False, False],
[False, False, False],
[True, True, True],
[True, True, True],
[False, False, False],
[True, True, True],
[False, False, False],
]


class IncarTest(PymatgenTest):
def setUp(self):
Expand Down