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 10 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
24 changes: 17 additions & 7 deletions pymatgen/io/vasp/inputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ 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,
Expand Down Expand Up @@ -122,12 +122,22 @@ def __init__(
"""
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
27 changes: 27 additions & 0 deletions pymatgen/io/vasp/tests/test_inputs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,33 @@ 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]))

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