search

libAtoms / ExtXYZ.jl

Extended XYZ read/write support for Julia
MIT License
13 stars 6 forks source link

Fix isapprox for dictionaries in testing (and highlight existing errors) #12

Closed jamesgardner1421 closed 3 years ago

jamesgardner1421 commented 3 years ago

It seems that the fixes yesterday for the data transposition worked for the positions but have broken the cell vectors. You can see this in the README example where the typo in the cell vectors Lattice="5.44 0.0 0.0 0.0 5.44 0.0 0.0 0.05.44" becomes 

"cell": [
      [
         5.44,
         0.0,
         0.0
      ],
      [
         0.0,
         5.44,
         0.05
      ],
      [
         0.0,
         0.0,
         0.44
      ]
   ]

whereas it should be

"cell": [
      [
         5.44,
         0.0,
         0.0
      ],
      [
         0.0,
         5.44,
         0.0
      ],
      [
         0.0,
         0.05,
         0.44
      ]
   ]

This issue was being hidden by the isapprox function which was causing the comparison to exit early before comparing all the fields. It would compare the sub-dictionary "arrays" and return true before comparing the cells.

So this pull request should cause the tests to fail and highlight that the cells are not being transformed correctly.

jameskermode commented 3 years ago

OK, thanks, I'll take a look. I was reassured also by the explicit comparisons with the ASE cell vectors, but obviously something is wrong.

jameskermode commented 3 years ago

OK, the transpose bug is real but the typo in Lattice is unrelated: for backwards compatibilty the Lattice is treated specially and assumed to be a 9-element vector in Fortran (i.e. same as Julia) column-major order. That's what leads to the odd result above, which must be due to an error in the underlying C parser (or even in the grammar). I'll open an issue in the https://github.com/libatoms/extxyz repo since the Python wrapper does the same thing:

$ pip3 install extxyz
...
$ python3
Python 3.7.4 (default, Mar 26 2020, 11:57:44)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import extxyz
>>> extxyz.read("test.xyz")
Atoms(symbols='Si8', pbc=True, cell=[[5.44, 0.0, 0.0], [0.0, 5.44, 0.0], [0.0, 0.05, 0.44]])

The other entries in the info are indeed incorrectly transposed, I'll fix that shortly. Row- vs column-major has thrown up all sorts of issues!

jameskermode commented 3 years ago

merging into a new branch so I can finish debugging.