Fix ValueError when passing selective_dynamics to Poscar

[chiang-yuan]

Before this fix, users could not directly add a selective_dynamics array to create a Poscar object:

poscar = Poscar(
    structure,
    selective_dynamics=boolean_array
)

and the code would produce an error:

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

A similar issue was reported previously about manually using add_site_property.

The minimal example reproducing the error and the bug fix is demonstrated in the attached notebook: poscar_debug.md

Summary

Include a summary of major changes in bullet points:

• Fixed the error where selective_dynamics boolean array is given for creating a Poscar object
• Applied similar bug fixes for velocities and predictor_corrector

[shyuep]

Pls provide a unit test to confirm the new functionality. Thanks.

[chiang-yuan]

Hi @shyuep, @janosh, I have added a unit test in test_files/vasp. Not sure if I put the file in the right place though. Let me know if this follows the guildlines. Thanks!

[janosh]

@chiang-yuan Thanks! The test should go into pymatgen/io/vasp/tests/test_inputs.py. There are some examples in there to learn from. Also, please check if you can reuse some of the existing POSCAR test files rather than adding new ones.

[chiang-yuan]

Thanks @janosh ! I followed the original style to test by comparing two primitive python lists and converted numpy array back to list to assign variables. Since the system can be large and comparing two nested python lists is somewhat tricky, (For example, all([[True, False],[True, True]]) will give True rather than False), I suggest in the future assigning and testing all these variable as numpy array. This also aligns with frac_coords, cart_coords in the core.structure module.

[janosh]

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

[chiang-yuan]

@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?

[janosh]

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.

[chiang-yuan]

Agree. Thanks @janosh !