search

aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
435 stars 188 forks source link

Importing `.xyz` structure with `ase` does not respect `pbc` #5967

Open mbercx opened 1 year ago

mbercx commented 1 year ago

Describe the bug

When trying to import an e.g. 2D structure from an .xyz file that correctly specifies the periodic boundary conditions (pbc), verdi data core.structure import ase does not respect pbc.

Steps to reproduce

(aiida-dev) mbercx@theospc46:~/envs/aiida-dev/data/structures$ more As-2D.xyz
2
Lattice="3.6086769266 0.0 0.0 -1.8043384633 3.1252058924 0.0 0.0 0.0 21.3114930844" pbc="True True False"
As           1.8043384632       1.0417352974      11.3518747709
As          -0.0000000002       2.0834705948       9.9596183135
(aiida-dev) mbercx@theospc46:~/envs/aiida-dev/data/structures$ verdi data core.structure import ase As-2D.xyz 
  Successfully imported structure As2 (PK = 16307)
(aiida-dev) mbercx@theospc46:~/envs/aiida-dev/data/structures$ verdi shell
Python 3.9.16 (main, Dec  7 2022, 01:11:58) 
Type 'copyright', 'credits' or 'license' for more information
IPython 8.12.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: load_node(16307).pbc
Out[1]: (True, True, True)

Expected behavior

The periodic boundary conditions should be respected, i.e. I'd expect:

In [1]: load_node(16307).pbc
Out[1]: (True, True, False)

Your environment

Additional context

Possibly related to https://github.com/aiidateam/aiida-core/issues/3476 Encountered while testing https://github.com/aiidateam/aiida-quantumespresso/pull/907

mbercx commented 1 year ago

Seems to be an ASE problem:

In [1]: from ase import io

In [2]: at = io.read('As-2D.xyz')

In [3]: at.pbc
Out[3]: array([ True,  True,  True])
mbercx commented 1 year ago

Opened a PR:

https://gitlab.com/ase/ase/-/merge_requests/2876

With those changes it seems to work fine:

(aiida-core) mbercx@theospc46:~/envs/aiida-core/data/structures$ verdi data core.structure import ase As-2D.xyz 

  Successfully imported structure As2 (PK = 1549)
(aiida-core) mbercx@theospc46:~/envs/aiida-core/data/structures$ verdi shell

Python 3.9.16 (main, Dec  7 2022, 01:11:58) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.34.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: load_node(1549).pbc
Out[1]: (True, True, False)

Once they merge and release we can update our dependencies and close this issue.

mbercx commented 1 year ago

Note that in case the pbc if differently (properly?) formatted:

Lattice="3.6086769266 0.0 0.0 -1.8043384633 3.1252058924 0.0 0.0 0.0 21.3114930844" pbc="T T F"

i.e. pbc="True True False" instead of pbc="T T F", the ase parsing works fine. So perhaps it's we who are to blame after all:

https://github.com/aiidateam/aiida-core/blob/fce1cd6d7da2b0241830cab30a83eb5ab978a16d/aiida/orm/nodes/data/structure.py#L1033-L1060

Also see https://gitlab.com/ase/ase/-/merge_requests/2876#note_1355561967

mbercx commented 1 year ago

Note: Seems the extxyz format should accept True/False as well for booleans, so nothing wrong with our exporter:

https://github.com/libAtoms/extxyz#logicalboolean

mbercx commented 1 year ago

My PR into ASE got merged, just waiting for a release now. Then we can update the version specifier and we should be good to go. 🚀