Fix floating point imprecision error in ordering property of CollinearMagneticStructureAnalyzer #3574
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ccount for floating point imprecision, added test for that.
…lose in favor abs(tot_mag) > 1e-8
janosh
approved these changes
Jan 23, 2024
There was a problem hiding this comment.
thanks @kaueltzen, esp. for adding a test right away! 👍 this looks great.
JaGeo
reviewed
Jan 23, 2024
| magnetic structure. Result is not guaranteed for correctness. | ||
| magnetic structure. Result is not guaranteed to be correct, just a best | ||
| guess. Tolerance for minimum total magnetization to be considered | ||
| ferro/ferrimagnetic is 1e-8. |
There was a problem hiding this comment.
I am wondering if we should make this adaptable when defining the instance the class if we are not sure about the tolerance yet.
There was a problem hiding this comment.
ah, wanted to add that but forgot. PRs welcome!
There was a problem hiding this comment.
@kaueltzen will hopefully open another PR as it was one of the questions in the PR!
janosh
added a commit
that referenced
this pull request
Jan 25, 2024
…zer in addition to PR #3574 (#3577) * Modified ordering property of CollinearMagneticStructureAnalyzer to account for floating point imprecision, added test for that. * improve CollinearMagneticStructureAnalyzer.ordering doc str, drop isclose in favor abs(tot_mag) > 1e-8 * compress tests/files/magnetic.example.CuO.mcif * add comment with source DOI for mcif file * link PR that added CuO AFM test * Added treshold_ordering for determination of magnetic ordering in CollinearMagneticStructureAnalyzer. * Added test case that demonstrates different ordering values dependent on threshold_ordering parameter. * document default threshold_ordering, refactor test --------- Co-authored-by: kueltzen <kueltzen@sv2218.zit.bam.de> Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The ordering property of the CollinearMagneticStructureAnalyzer class is set depending on the
total_magnetization = abs(sum(magmoms)). Due to floating point imprecision, the sum of magmoms can adapt values close to zero instead of zero. This can, for example, lead to collinear AFM structures being misclassified as FiM.This was resolved by replacing
if total_magnetization > 0withif not isclose(total_magnetization, 0, abs_tol=1e-8).A test to determine the magnetic ordering of MAGNDATA entry 1.62 (CuO) which would have failed with the previous code was added.
Source mcif:
https://www.cryst.ehu.es/magndata/index.php?index=1.62
doi: 10.1088/0022-3719/21/15/023
Open questions
@janosh
abs_tol=1e-8a reasonable choice for "being close to zero" here?