materialsproject / emmet

Be a master builder of databases of material properties. Avoid the Kragle.
https://materialsproject.github.io/emmet/
Other
51 stars 63 forks source link

Bug: The `.as_dict()` method on `CrystalSystem` is not Monty compatible #914

Open Andrew-S-Rosen opened 6 months ago

Andrew-S-Rosen commented 6 months ago

Problem

from emmet.core.symmetry import CrystalSystem
sym = CrystalSystem("Triclinic")
sym.as_dict()

This returns the string "Triclinic" rather than a dictionary. This would normally be fine, but the fact that it's a string means that doing dumpfn doesn't work on a data structure containing a CrystalSystem object.

from monty.serialization import dumpfn

dumpfn(sym, "test.json")

Traceback:

------------------------------------------------------------------------
TypeError                              Traceback (most recent call last)
Cell In[47], [line 1](vscode-notebook-cell:?execution_count=47&line=1)
----> [1](vscode-notebook-cell:?execution_count=47&line=1) dumpfn(CrystalSystem("Triclinic"),"test.json")

File [c:\Users\asros\miniconda\envs\zeolites\lib\site-packages\monty\serialization.py:123](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/serialization.py:123), in dumpfn(obj, fn, fmt, *args, **kwargs)
    [121](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/serialization.py:121)     if "cls" not in kwargs:
    [122](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/serialization.py:122)         kwargs["cls"] = MontyEncoder
--> [123](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/serialization.py:123)     fp.write(json.dumps(obj, *args, **kwargs))
    [124](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/serialization.py:124) else:
    [125](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/serialization.py:125)     raise TypeError(f"Invalid format: {fmt}")

File [c:\Users\asros\miniconda\envs\zeolites\lib\json\__init__.py:238](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:238), in dumps(obj, skipkeys, ensure_ascii, check_circular, allow_nan, cls, indent, separators, default, sort_keys, **kw)
    [232](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:232) if cls is None:
    [233](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:233)     cls = JSONEncoder
    [234](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:234) return cls(
    [235](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:235)     skipkeys=skipkeys, ensure_ascii=ensure_ascii,
    [236](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:236)     check_circular=check_circular, allow_nan=allow_nan, indent=indent,
    [237](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:237)     separators=separators, default=default, sort_keys=sort_keys,
--> [238](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/__init__.py:238)     **kw).encode(obj)

File [c:\Users\asros\miniconda\envs\zeolites\lib\json\encoder.py:199](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:199), in JSONEncoder.encode(self, o)
    [195](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:195)         return encode_basestring(o)
    [196](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:196) # This doesn't pass the iterator directly to ''.join() because the
    [197](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:197) # exceptions aren't as detailed.  The list call should be roughly
    [198](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:198) # equivalent to the PySequence_Fast that ''.join() would do.
--> [199](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:199) chunks = self.iterencode(o, _one_shot=True)
    [200](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:200) if not isinstance(chunks, (list, tuple)):
    [201](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:201)     chunks = list(chunks)

File [c:\Users\asros\miniconda\envs\zeolites\lib\json\encoder.py:257](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:257), in JSONEncoder.iterencode(self, o, _one_shot)
    [252](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:252) else:
    [253](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:253)     _iterencode = _make_iterencode(
    [254](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:254)         markers, self.default, _encoder, self.indent, floatstr,
    [255](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:255)         self.key_separator, self.item_separator, self.sort_keys,
    [256](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:256)         self.skipkeys, _one_shot)
--> [257](file:///C:/Users/asros/miniconda/envs/zeolites/lib/json/encoder.py:257) return _iterencode(o, 0)

File [c:\Users\asros\miniconda\envs\zeolites\lib\site-packages\monty\json.py:394](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/json.py:394), in MontyEncoder.default(self, o)
    [391](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/json.py:391)     d = o.as_dict()
    [393](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/json.py:393) if "@module" not in d:
--> [394](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/json.py:394)     d["@module"] = str(o.__class__.__module__)
    [395](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/json.py:395) if "@class" not in d:
    [396](file:///C:/Users/asros/miniconda/envs/zeolites/lib/site-packages/monty/json.py:396)     d["@class"] = str(o.__class__.__name__)

TypeError: 'str' object does not support item assignment

Proposed Solution

Modify the .as_dict() method?

Alternatives

No response

jmmshn commented 5 months ago

Looks like I ran into something similar, resolved on monty's side: https://github.com/materialsvirtuallab/monty/pull/602/

Andrew-S-Rosen commented 5 months ago

Oh that was smart! Thanks!! I'll check if is resolved with the monty update.

Andrew-S-Rosen commented 4 months ago

Looks like your PR didn't fix this one btw, @jmmshn. I'll have to dig into it later.

jmmshn commented 4 months ago

So I think the custom as_dict for this takes precedence over the new general for the Enums

So something like this should work.

from monty.serialization import dumpfn
from enum import Enum
class Test(Enum):
    a = "a"
    b = "b"
t = Test.a
dumpfn(t, "test.json")

So I think if we just remove the custom as_dict and from_dict for that class then things should work. But we will have a see about the consequences.

Andrew-S-Rosen commented 4 months ago

Thanks for the pointer, @jmmshn! Removing the .as_dict() method in emmet.core.utils.ValueEnum results in an AttributeError:

AttributeError: 'CrystalSystem' object has no attribute 'as_dict'

I'll dig into this later in the week. I think I know where to look now. 👍

Andrew-S-Rosen commented 3 months ago

@munrojm can you please re-open this? It won't let me do so. It needs to be reopened due to #974.