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.

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 isapprox for dictionaries in testing (and highlight existing errors) #12

Merged
merged 1 commit into from
Aug 20, 2021

Conversation

jamesgardner1421
Copy link
Contributor

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
Copy link
Member

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.

@@ -12,8 +12,8 @@ function Base.isapprox(seq1::AbstractDict, seq2::AbstractDict)
for (k1,v1) in seq1
k1 ∈ keys(seq2) || (println("key $k1 missing from seq2"); return false)
if v1 isa AbstractDict
return isapprox(v1, seq2[k1])
Copy link
Member

@jameskermode jameskermode Aug 20, 2021

Choose a reason for hiding this comment

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

with this change we no longer recursively check sub-dictionaries. Isn't that a problem for arrays and info too? It should probably be something like isapprox(v1, seq2[k1]) || return false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think and isapprox are equivalent, so the new line is still recursively calling this function. I just changed the syntax to be more consistent with the rest of the function.

@jameskermode
Copy link
Member

jameskermode commented Aug 20, 2021

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
Copy link
Member

merging into a new branch so I can finish debugging.

@jameskermode jameskermode merged commit 581116e into libAtoms:fix-tesing Aug 20, 2021
jameskermode added a commit that referenced this pull request Aug 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants