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

Raise ValueError in SpacegroupAnalyzer.get_symmetrized_structure() if spglib returns no symmetries #2724

Merged
merged 5 commits into from Nov 7, 2022

Conversation

janosh
Copy link
Member

@janosh janosh commented Nov 7, 2022

Closes #2723.

@shyuep
Copy link
Member

shyuep commented Nov 7, 2022

I think we need to be clear what spglib not returning symmetries mean. If it is a problem assigning any symmetry at all, then it should be an error. If it is a P1 structure, then it should return a single symmetry operation - the identity.

@shyuep
Copy link
Member

shyuep commented Nov 7, 2022

Also, let's try to keep linting fixes separate from code fixes unless they are in the same file. This PR has 24 files changed and it is not possible to see what is relevant easily.

@materialsproject materialsproject deleted a comment from coveralls Nov 7, 2022
@janosh
Copy link
Member Author

janosh commented Nov 7, 2022

I think we need to be clear what spglib not returning symmetries mean. If it is a problem assigning any symmetry at all, then it should be an error. If it is a P1 structure, then it should return a single symmetry operation - the identity.

It's the former. I think the structure should have spacegroup 225 (cF4). That's what I get when I decrease symprec=1e-1 to e.g. 1e-2. What type of error? ValueError?

Also, let's try to keep linting fixes separate from code fixes unless they are in the same file. This PR has 24 files changed and it is not possible to see what is relevant easily.

Good point. I'll try to accumulate them over time into separate PRs.

Although you can see the relevant changes by clicking on the commits with non-formatting messages like fb2fc72.

@shyuep
Copy link
Member

shyuep commented Nov 7, 2022

If it is the former, you can have a ValueError that says spglib cannot determine symmetry and you may need to change the symprec. Unfortunately symmetry determination is always precision specific. Nothing much we can do on that front.

For linting fixes, you can directly push to master branch and there is no need to create PRs for it.

@janosh janosh enabled auto-merge November 7, 2022 23:46
@janosh janosh disabled auto-merge November 7, 2022 23:54
@janosh janosh merged commit ffbebce into master Nov 7, 2022
@janosh janosh deleted the gh-2723 branch November 7, 2022 23:54
@janosh janosh changed the title Fix TypeError in SpacegroupAnalyzer.get_symmetrized_structure() if spglib returns no symmetries Raise ValueError in SpacegroupAnalyzer.get_symmetrized_structure() if spglib returns no symmetries Nov 8, 2022
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.

SpacegroupAnalyzer.get_symmetrized_structure() raises TypeError
2 participants