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

Added threshold_ordering parameter to CollinearMagneticStructureAnalyzer in addition to PR #3574 #3577

Merged
merged 10 commits into from
Jan 25, 2024

Conversation

kaueltzen
Copy link
Contributor

Summary

A parameter threshold_ordering was added to CollinearMagneticStructureAnalyzer below which the total magnetization is treated as zero when defining the magnetic ordering.

@JaGeo @janosh self.total_magmoms = sum(magmoms) and self.magnetization = sum(magmoms) / structure.volume are defined before processing the input magmoms. They could also be affected by the error described in PR #3574 - do you think it's a good idea to add a threshold here as well?

@janosh
Copy link
Member

janosh commented Jan 24, 2024

They could also be affected by the error described in PR #3574 - do you think it's a good idea to add a threshold here as well?

what would change based on the threshold? setting self.total_magmoms and self.magnetization to exactly 0 when below the threshold?

could you add a test case that passes a custom threshold_ordering and verifies different behavior?

@janosh janosh added enhancement A new feature or improvement to an existing one magmoms Magnetism related needs testing PRs that are not ready to merge due to lacking tests labels Jan 24, 2024
@kaueltzen
Copy link
Contributor Author

They could also be affected by the error described in PR #3574 - do you think it's a good idea to add a threshold here as well?

what would change based on the threshold? setting self.total_magmoms and self.magnetization to exactly 0 when below the threshold?

Yes - if, for example, a database is scanned for structures with zero magnetization based on self.magnetization the imprecision issue could be relevant. but i don't know if this important enough to be included?

@JaGeo
Copy link
Member

JaGeo commented Jan 24, 2024

They could also be affected by the error described in PR #3574 - do you think it's a good idea to add a threshold here as well?

what would change based on the threshold? setting self.total_magmoms and self.magnetization to exactly 0 when below the threshold?

Yes - if, for example, a database is scanned for structures with zero magnetization based on self.magnetization the imprecision issue could be relevant. but i don't know if this important enough to be included?

I would not change the data that is stored on the object. I think this would be unexpected behavior. If someone needs this, they will implement it themself

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks @kaueltzen! 👍

@janosh janosh merged commit cb2f490 into materialsproject:master Jan 25, 2024
20 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A new feature or improvement to an existing one magmoms Magnetism related needs testing PRs that are not ready to merge due to lacking tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants