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

Symmetrization failed #4

Closed
mkhorton opened this issue May 12, 2020 · 7 comments · Fixed by #6
Closed

Symmetrization failed #4

mkhorton opened this issue May 12, 2020 · 7 comments · Fixed by #6

Comments

@mkhorton
Copy link

Hello,

Thanks for the excellent code and paper, which is very interesting.

I was trying out Auguste and it seems to work on 99% of cells but I sometimes receive a symmetrization failed error. Is it expected that symmetrization would fail in some cases?

For example, consider this cell:

[[-5.35538034 -0.1350599  -5.2795042 ]
 [ 5.35538034 -5.2795042  -0.1350599 ]
 [-5.35538034 -5.2795042  -0.1350599 ]]

Separately, the pip install doesn't work on macOS without manually modifying the setup.py to set MACOSX_DEPLOYMENT_TARGET.

Many thanks!

@pmla
Copy link
Owner

pmla commented May 12, 2020

Thanks for reporting this. It is a silly bug in the Minkowski reduction code. I am working on a fix.

@pmla
Copy link
Owner

pmla commented May 12, 2020

I have fixed the bug. I have also added a more specific error message so this type of bug can be more easily identified in future.

pmla added a commit that referenced this issue May 12, 2020
@mkhorton
Copy link
Author

Many thanks Peter!

With the most recent version just pushed, the cell above is now fixed, but I do get a Minkowski reduction error with this cell:

cell = [
    [-4.1855226,   4.17604824,  0.02341527],
    [ 4.1855226,   0.02341527,  4.17604824],
    [ 4.1855226,   4.17604824,  0.02341527]
]

These were the only two cells I could find that failed.

@pmla
Copy link
Owner

pmla commented May 12, 2020

Thanks for the feedback. I will do some more testing. This is clearly no good!

@pmla
Copy link
Owner

pmla commented May 14, 2020

It should be resolved now.

There were two bugs:

  • The C round function does not behave in the same way as python's round function. C rounds away from zero, whereas python rounds to the nearest even number.
  • Even with proper rounding there is a risk of entering a cycle. I have added cycle checking, and pushed it upstream to ASE, where I ported this code from.

@mkhorton
Copy link
Author

Many thanks Peter!

Regarding the reduction, is there a reason it has to be a Minkowski reduction? We have LLL reduction implemented ourselves, I'm just wondering if it would be worthwhile for us to add Minkowski (I'm not sure when one might need to use one over the other or what the distinction is, other than that LLL is fast, and that perhaps Minkowski is more strict(?)).

@pmla
Copy link
Owner

pmla commented May 14, 2020

Minkowski reduction describes the lattice with the shortest possible set of vectors. LLL is less strict, guaranteeing only that:

The first vector in the basis cannot be much larger than the shortest non-zero vector

Minkowski reduction is pretty handy for other things too. If you want to find the nearest neighbours of an atom, you can restrict the search to the 27 adjacent cells of a Minkowski-reduced cell. I recently changed ASE's neighborlist to use this approach - it fixed some buggy behaviour and speeded it up.

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 a pull request may close this issue.

2 participants