Fix MagOrderingTransformation by enforcing implicit-zero magnetic moments in SpaceGroupAnalyzer

[mattmcdermott]

This PR addresses the bugs described in #3006. To summarize: changes to default magnetic moments introduced in #2727 now mean that structures with only partially defined magnetic moments (e.g., on half the sites) cannot be successfully analyzed by SpaceGroupAnalyzer. This was encountered when performing magnetic ordering enumeration, as the previous default behavior for MagOrderingTransformation does not implicitly yield spins of 0 on the nonmagnetic sites. Altogether this has been fixed with two edits:

• Fix 1: Introduce logic in SpaceGroupAnalyzer. If any site in the structure has magnetic moments defined, then assume that any undefined ones are implicitly zero.
• Fix 2: Change the default behavior of MagOrderingTransformation to ensure that all the sites in the transformed structure have labeled spins (this means explicitly defining spin=0), per recommendation by @mkhorton

Todos

• Still need to write tests!

Checklist

• Google format doc strings added. Check with ruff.
• Type annotations included. Check with mypy.
• Tests added for new features/fixes.
• All tests and linting pass.

[mattmcdermott]

This PR has been open for a while and fixed some of the previous magnetic moment problems. I apologize for not adding tests yet to get it merged.

However, with the recent changes in Species properties (related to spin), theMagOrderingTransformation code is again broken. I don't really understand what happened/changed, and now I am not sure I have the bandwidth to try fixing this again.

Does anyone with more knowledge of this situation able to address? @janosh @shyuep @mkhorton

[janosh]

Just for clarity, this PR still addresses a relevant bug and is just missing tests? Sounds like the MagOrderingTransformation breaking again is a different problem?

[mattmcdermott]

The original problem this PR addressed is still "fixed"(i.e., enforcing implicit-zero magnetic moments) but the implementation here will need to be reworked to account for the new defaults in how spin is handled.

I am just somewhat confused about specifically why it's broken now and what is the right thing to do... I think @shyuep also expressed that sentiment: 9d0461f

[janosh]

I am just somewhat confused about specifically why it's broken now and what is the right thing to do... I think @shyuep also expressed that sentiment: 9d0461f

Lol! 😄

Well, I'm happy to merge as is if this is at least a step towards improvement.

[mkhorton]

Well it certainly worked at some point! I think keeping this running in CI was tricky because of its dependence on `enumlib`, and since `enumlib` on `conda` was quite out of date (this is not to blame the issue on `enumlib`, simply for why this bug might have remained uncaught).

[shyuep]

This fix actually works. I am merging.

[shyuep]

I should add that my changes to Specie does not do anything backward incompatible. I was deliberate in making sure all existing code still works. @mattmcdermott 's solution works since it is based on existing ways of determining spin.

[shyuep]

Also, no tests are necessary. There is an existing test in MagTransformation. The tests fail previously. Not sure why the Github actions is not detecting it. But I confirm the tests pass now.

[mattmcdermott]

Oh, that is great to hear! Glad you were able to pull this in, @shyuep. Thank you for investigating :)

I should have spent some more time digging yesterday before I brought up that it was broken; it is actually a different section of code that is broken now -- specifically CollinearMagneticStructureAnalyzer. This seems only to be the case when a structure is provided that has a defined spin property but not defined magmoms. Seems to be something that is missed by our current tests.

I just found a simple one-line fix for that, so I will open up a new PR :) thanks again everyone!