materialsproject / pymatgen

Python Materials Genomics (pymatgen) is a robust materials analysis code that defines classes for structures and molecules with support for many electronic structure codes. It powers the Materials Project.
https://pymatgen.org
Other
1.48k stars 851 forks source link

Spglib unable to obtain structure space group info, during magnetic structure enumeration #3006

Open kamronald opened 1 year ago

kamronald commented 1 year ago

When making an instance of MagneticStructureEnumerator, there is an error with the SpacegroupAnalyzer in determining space group number of structures enumerated using EnumerateStructureTransformation. I used the following code. Also attached are the screenshot with the errors and the CONTCAR used to generate magnetic orderings.

from pymatgen.core import Structure
from pymatgen.analysis.magnetism import MagneticStructureEnumerator
struc = Structure.from_file('CONTCAR')

mag_enum = MagneticStructureEnumerator(structure=struc, default_magmoms={'Li': 0, 'Mn': 4, 'O': 0}, strategies=('ferromagnetic', 'antiferromagnetic'),
                                       truncate_by_symmetry=True, transformation_kwargs={'min_cell_size': 1, 'max_cell_size': 1})

limno2_contcar.zip

kamronald commented 1 year ago

@mkhorton, @guymoore13 have you guys seen this error before, and know a potential solution?

mkhorton commented 1 year ago

First thing I would check is your spglib version. What version are you running? This recently saw a 2.0 release and had a few bugs which were fixed with the latest version.

kamronald commented 1 year ago

I'm using spglib 2.0.2

janosh commented 1 year ago

For additional context, @kamronald and I sat down and checked all the obvious things. It's not environment dependent and the error occurs using both the latest v1 and v2 release of spglib. On the other hand, calling struc.get_space_group_info() directly on the Structure loaded from @kamronald's CONTCAR works and gives expected space group 227. So must be due to sth happening inside MagneticStructureEnumerator where I'm out of my depth.

janosh commented 1 year ago

Here's a complete repro:

import os

from pymatgen.analysis.magnetism import MagneticStructureEnumerator
from pymatgen.core import Structure

dirname = os.path.dirname(__file__)
struct = Structure.from_file(f"{dirname}/LiMnO2-CONTCAR")

struct.get_space_group_info()  # works

mag_enum = MagneticStructureEnumerator(
    structure=struct,
    default_magmoms={"Li": 0, "Mn": 4, "O": 0},
    strategies=("ferromagnetic", "antiferromagnetic"),
    truncate_by_symmetry=True,
    transformation_kwargs={"min_cell_size": 1, "max_cell_size": 1},
)

None of the MagneticStructureEnumerator kwargs affect the error.

guymoore13 commented 1 year ago

If I remember correctly, I've had similar issues with spglib a while back, which I believe that Matt resolved.

I can take a look at the issue later today, and/or you're welcome to stop by my cubicle if that's easier! Thank you for bringing this to our attention.

guymoore13 commented 1 year ago

In my local build, I encounter the error ValueError: Unable to enumerate. Is this the same error thrown for you both?

guymoore13 commented 1 year ago

@kamronald stopped by, and we resolved the issue - at least on my machine!

He identified that enumlib was the source of the error (not the interface to spglib), so I updated and re-built the enum.x and makestr.x executables. Afterwards MagneticStructureEnumerator worked for me.

@janosh, what do you think would be the best way to either (a) make this more clear to users, or (b) update the Python dependencies? I mention (b) because it appears that enumlib can be installed via conda, which may be a new feature.

janosh commented 1 year ago

@guymoore13 That's great!

What exactly solved the error? Updating to a newer version of enumlib? If so, ideally pymatgen would check the version and prompt users to update if outdated.

guymoore13 commented 1 year ago

@janosh Yes, I simply pulled the latest enumlib, and then built the corresponding fortran executables. I'm not sure if it's possible to identify the enumlib version from the executable(s) alone, which is why I suggested the conda install. However, I have not tried installing/building enumlib this way yet.

kamronald commented 1 year ago

I just tried installing enumlib using conda, but it EnumerateStructureTransformation throws the that Guy saw “ValueError: Unable to enumerate”. Working on installing enumlib manually right now.

On Thu, May 25, 2023 at 1:43 PM Guy Moore @.***> wrote:

@janosh https://github.com/janosh Yes, I simply pulled the latest enumlib, and then built the corresponding fortran executables. I'm not sure if it's possible to identify the enumlib version from the executable(s) alone, which is why I suggested the conda install. However, I have not tried installing/building enumlib this way yet.

— Reply to this email directly, view it on GitHub https://github.com/materialsproject/pymatgen/issues/3006#issuecomment-1563483910, or unsubscribe https://github.com/notifications/unsubscribe-auth/AISIE7HX2EBDEYPI3FQKONTXH676FANCNFSM6AAAAAAYLDY6SE . You are receiving this because you were mentioned.Message ID: @.***>

-- Ronald Kam Graduate Student Researcher | Ceder Group Dept. of Materials Science and Engineering University of California, Berkeley

guymoore13 commented 1 year ago

@kamronald You may want to check whether or not you're calling the correct executables, i.e. from a previously compiled enumlib vs. the conda package.

guymoore13 commented 1 year ago

Ah, from the enumlib README it appears that their conda install only works for OSX and Linux, so it may be a safer bet to simply update the error thrown during calls to enumlib from EnumerateStructureTransformation, e.g. something like: "Error running enumlib executable. Please make sure that enum.x and makestr.x executables are up-to-date." ?

janosh commented 1 year ago

Cool, looks like the conda release was prompted by @shyamd no less. 😄

mkhorton commented 1 year ago

Thanks all for debugging. Yes, I think the version on conda is quite old. I’ve always built from source myself (luckily this is fairly straight forward).

I also briefly maintained a homebrew formula to install: https://github.com/mkhorton/homebrew-matsci/blob/master/Formula/enumlib.rb However this too would need updating.

janosh commented 1 year ago

it may be a safer bet to simply update the error thrown during calls to enumlib from EnumerateStructureTransformation, e.g. something like: "Error running enumlib executable. Please make sure that enum.x and makestr.x executables are up-to-date." ?

Happy to take a PR for this!

guymoore13 commented 1 year ago

@janosh, yes this was by no means intended as a request!

FYI, it appears that @kamronald is still encountering issues besides the enumlib issue that we resolved.

mattmcdermott commented 1 year ago

@kamronald @janosh @guymoore13 Funny enough, I just ran into the same error originally posted by Ronald. I had installed enumlib through conda. However, compiling enumlib from scratch did not fix the problem.

So I did some experimenting with different pymatgen versions and was able to get rid of the error by downgrading to pymatgen==2022.11.1. Note that pymatgen v2022.11.7 does not work, so that might narrow down what is causing the issue.

Note that if you try this yourself you will also have to downgrade to numpy==1.19.5 due to an error about np.int being deprecated in 1.20 onwards. (You might also have to downgrade other things that require numpy>=1.20 like matplotlib).

guymoore13 commented 1 year ago

@mattmcdermott Thank you for this information, I suspected as much. @kamronald is having issues with the ground-state enumeration with the latest pymatgen. It works for me, but I've been sticking with pymatgen==2022.0.17 and numpy==1.21.4!

kamronald commented 1 year ago

Thanks all for the ideas! So far @mattmcdermott 's method of using pymatgen v2022.11.7 is able to remove all the errors raised. However the enumerated structures do not match the original one, and do not seem very physical at all. I have attached them below. I'll look into it more but it's perhaps an issue with finding the primitive cell, which is performed during the enumeration (pymatgen.analysis.magnetism.analyzer.MagneticStructureEnumerator._generate_ordered_structures) mag_enum.zip

mattmcdermott commented 1 year ago

I think I finally figured this one out. The enumerated structures produced within MagneticStructureEnumerator contain only spins on SOME sites (i.e., the magnetic species). See example, which I dumped within the method:

Structure Summary
Lattice
    abc : 6.467801905019046 9.83025604876038 6.60018016898948
 angles : 81.6316987446084 105.45156612496812 102.22794899018017
 volume : 393.5240108113061
      A : 5.647798 3.151958 0.0
      B : -5.647798 5.847555 5.526519
      C : 0.0 -3.6083190000000003 5.526519
    pbc : True True True
PeriodicSite: V,spin=5 (-1.1445, 5.1737, 4.1449) [0.4730, 0.6757, 0.0743]
PeriodicSite: V,spin=-5 (4.5033, 2.0217, 4.1449) [0.9730, 0.1757, 0.5743]
PeriodicSite: V,spin=-5 (1.1445, 0.2175, 6.9081) [0.5270, 0.3243, 0.9257]
PeriodicSite: V,spin=5 (-4.5033, 3.3695, 6.9081) [0.0270, 0.8243, 0.4257]
PeriodicSite: Bi (-4.4548, 1.5653, 9.6714) [0.0334, 0.8222, 0.9278]
PeriodicSite: Bi (1.1930, 2.0217, 4.1449) [0.5334, 0.3222, 0.4278]
PeriodicSite: Bi (4.4548, 3.8259, 1.3816) [0.9666, 0.1778, 0.0722]
PeriodicSite: Bi (-1.1930, 3.3695, 6.9081) [0.4666, 0.6778, 0.5722]
PeriodicSite: O (0.2406, 1.6711, 6.3422) [0.4929, 0.4503, 0.6973]
PeriodicSite: O (0.2406, 7.9750, 6.3422) [0.9929, 0.9503, 0.1973]
PeriodicSite: O (0.2406, 2.3723, 1.9476) [0.3210, 0.2784, 0.0740]
PeriodicSite: O (0.2406, 5.0679, 7.4741) [0.8210, 0.7784, 0.5740]
PeriodicSite: O (-0.2406, 3.7201, 4.7109) [0.5071, 0.5497, 0.3027]
PeriodicSite: O (-0.2406, -2.5839, 4.7109) [0.0071, 0.0497, 0.8027]
PeriodicSite: O (-0.2406, 3.0189, 9.1054) [0.6790, 0.7216, 0.9260]
PeriodicSite: O (-0.2406, 0.3233, 3.5789) [0.1790, 0.2216, 0.4260]
PeriodicSite: O (2.1649, -0.1190, 5.5923) [0.5677, 0.1843, 0.8276]
PeriodicSite: O (-3.4829, 3.0330, 5.5923) [0.0677, 0.6843, 0.3276]
PeriodicSite: O (2.1649, 4.1624, 2.6975) [0.7573, 0.3740, 0.1141]
PeriodicSite: O (-3.4829, 3.7060, 8.2240) [0.2573, 0.8740, 0.6141]
PeriodicSite: O (-2.1649, 5.5102, 5.4608) [0.4323, 0.8157, 0.1724]
PeriodicSite: O (3.4829, 2.3582, 5.4608) [0.9323, 0.3157, 0.6724]
PeriodicSite: O (-2.1649, 1.2288, 8.3555) [0.2427, 0.6260, 0.8859]
PeriodicSite: O (3.4829, 1.6852, 2.8290) [0.7427, 0.1260, 0.3859]

With @lbluque's PR #2727 in November (i.e., pymatgen 2022.11.7), this means that SpaceGroupAnalyzer no longer assumes a default magmom of 0 on the unlabeled sites. So a smaller list of magmoms [5,-5,-5,5] is being passed to spglib and that is causing it to break and return nothing, making SpaceGroupAnalyzer._space_group_data empty.

I fixed this locally by reverting the changes in this commit: https://github.com/materialsproject/pymatgen/pull/2727/commits/d39aa9fa3fab8a261c18bfc738b1cfe94cc0ccae

Since I don't really understand the implications or why this was originally changed, maybe @lbluque @mkhorton or @janosh can comment on what we should do here?

mkhorton commented 1 year ago

Ah, that explains it. I apologise because I was aware of the issue of untagged (implicit zero) spins, because this can have other consequences too: for example, incorrect (undesired) orderings if, say, a VASP input set assumes a non-zero magmom if not otherwise specified. I did not anticipate the connection to SpacegroupAnalyzer.

The good news, I hope, is that this is an easy fix! We remove the implicit zero spins and make sure they’re explicitly specified instead.

I think the original thinking behind not specifying an explicit zero is that the fallback default magmom (0.6) would not be used for other sites, which I originally thought was set for stability reasons, but having had more experience here I think the correct default/fallback magmom could probably safely be exactly 0 instead.

lbluque commented 1 year ago

I updated to remove implicit zero spins in #2727 following the conversation in #2725 about (breaking) changes in spglib > 2.0. also this comment.

It seems now the solution is to handle cases where we need implicit zero magmoms to be set explicitly to zero (this issue), and cases where we actually do not want them at all to only obtain unique (ignoring time reversal) symmetry operations (issue #2725).

I had originally suggested including a unique or equivalent keyword in SpacegroupAnalyzer.get_symmetry_operations, but perhaps can also set explicit zero spin if any site has a set spin value.

lbluque commented 1 year ago

(also apologies that this has caused such a long-running detective effort!)

janosh commented 1 year ago

(also apologies that this has caused such a long-running detective effort!)

Such is the nature of large code bases. They often involve a bit of whack-a-mole. 🐰 😄

mattmcdermott commented 1 year ago

It seems now the solution is to handle cases where we need implicit zero magmoms to be set explicitly to zero (this issue), and cases where we actually do not want them at all to only obtain unique (ignoring time reversal) symmetry operations (issue #2725).

This context helps a ton. Thanks @lbluque.

perhaps can also set explicit zero spin if any site has a set spin value.

I like this idea for the fix; seems very logical to me!

(also apologies that this has caused such a long-running detective effort!)

Don't worry, it actually wasn't so bad. The hardest part was the lack of error message provided by the spglib call!

mattmcdermott commented 1 year ago

Does this fix make sense to you? https://github.com/mattmcdermott/pymatgen/commit/1e303a384940adc2f3975fa3f1daf6a8be397c1a

kamronald commented 1 year ago

@mattmcdermott Yes this makes sense! And thanks to @lbluque and @mkhorton for clarifying those earlier changes to SpacegroupAnalyzer!

kamronald commented 1 year ago

I have tried implementing @mattmcdermott's changes, and now can make a MagneticStructureEnumerator instance without any raised errors!

However the enumerated structures do not match the original one, and do not seem very physical at all.

However, the other issue that I brought up earlier about the enumerated structures being different from the original structure and being highly unphysical (ie O-O bonds being 0.3 A) still persists. :( This is from using the latest pymatgen version 2023.5.31, spglib 2.0.2, and enumlib 2.0.6. I have tried this with a different structure (attached below) and the issue remains. To reproduce this: struc = Structure.from_file('limno2_ortho_mp_18767.cif') mag_enum = MagneticStructureEnumerator(structure=struc, default_magmoms={'Li': 0, 'Mn': 4, 'O': 0}, strategies=['ferromagnetic', 'antiferromagnetic'])

limno2_ortho_mp_18767.cif.zip

mattmcdermott commented 1 year ago

@kamronald Oh yeah that looks pretty bad. There is definitely more to this bug than just what I fixed.

mkhorton commented 1 year ago

Progress, at least. Are you using enumlib’s makeStr.py or makestr.x here? Perhaps a bug in this interface? (The former is the newer one)

mkhorton commented 1 year ago

Just to be clear, the correct fix here (in addition to the SGA change) is to also ensure spin=0 is set on all relevant atoms on coming out of the MagneticStructureEnumerator. All atoms in structures generated by this class should have a spin defined; the implicit zero spin was a bad call on my part originally.

kamronald commented 1 year ago

Progress, at least. Are you using enumlib’s makeStr.py or makestr.x here? Perhaps a bug in this interface? (The former is the newer one)

@mkhorton Yes I am using 'makestr.x', I'll look into other potential errors with enumlib

mattmcdermott commented 1 year ago

@kamronald and I figured out yesterday that the enumerator appears to be working, but actually CifWriter is causing bizarre issues exporting these structures to CIF (or MCIF) files. It seems to be related to the "spin" attribute (the usual magmom site property is fine). Any thoughts on this @mkhorton?

mattmcdermott commented 1 year ago

@mkhorton I guess more broadly, what is the purpose of using the "spin" property for species rather than just the magmom site property? Not sure I really know the context behind this and it does seem to cause some headaches to account for both (e.g., like with this CifWriter bug)

kamronald commented 1 year ago

To add on, a spin is only added to the species if it is nonzero. Was there a reason for not assigning 0 spin to a species?

mattmcdermott commented 1 year ago

@kamronald I think Matt already mentioned that in a previous reply; I just added a commit that changes the default behavior so that all non-labeled sites now explicitly have spin 0 defined.

https://github.com/mattmcdermott/pymatgen/commit/e2a38410bcfe6a41101cd586adb3979731cd2060

kamronald commented 1 year ago

@mattmcdermott Oh right, thanks