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

Do not introduce zero magmoms in SpacegroupAnalyzer #2727

Merged
merged 3 commits into from Nov 8, 2022

Conversation

lbluque
Copy link
Contributor

@lbluque lbluque commented Nov 8, 2022

Summary

Do not introduce zero magmoms in SpacegroupAnalyzer following changes in spglib >= 2. See #2725

TODO

Checklist

Work-in-progress pull requests are encouraged, but please put [WIP]
in the pull request title.

Before a pull request can be merged, the following items must be checked:

  • Doc strings have been added in the Google docstring format.
    Run pydocstyle on your code.
  • Type annotations are *highly- encouraged. Run mypy path/to/file.py to type check your code.
  • Tests have been added for any new functionality or bug fixes.
  • All linting and tests pass.

Note that the CI system will run all the above checks. But it will be much more efficient if you already fix most
errors prior to submitting the PR. We highly recommended installing pre-commit hooks. Simply pip install -U pre-commit && pre-commit install in the repo's root directory. Afterwards linters will run before every commit and abort if any issues pop up.

@coveralls
Copy link

coveralls commented Nov 8, 2022

Coverage Status

Coverage remained the same at 0.0% when pulling 803d750 on lbluque:master into d17a0f4 on materialsproject:master.

modify sga._cell in place to make sure ValueError is raised
@janosh
Copy link
Member

janosh commented Nov 8, 2022

This is great! Thanks @lbluque. Just tried out your changes on another 5 structures for which spglib with 0 magmons gave me no symmetries. This fix works for all of them!

I think it might still be useful to keep the test that raises ValueError on the Co8 structure I added in #2724. Just so we keep the nicer error message in case a similar problem reappears in the future. We can just manually insert magmoms = [0] * len(struct) in the test _cell to still make it raise the ValueError.

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.

None yet

3 participants