materialsvirtuallab/monty

jsanitize does not use .as_dict() on MSONables unless strict=True

Andrew-S-Rosen opened this issue · 3 comments

When using monty.json.jsanitize, it does not use the .as_dict() method on MSONable objects unless strict=True.

While one can use strict=True to force the MSONable serialization, there are many cases where the user may want the input object to be converted to a str (rather than having an exception thrown) if it's of a type that jsanitize does not "understand" while still benefiting from the .as_dict() conversion of MSONables. After all, the docstring does say "also recursively encodes all objects using Monty's as_dict() protocol," which is not really the case unless you force strict=True.

It can be traced back to this line:

monty/monty/json.py

Lines 502 to 503 in 34493ab

if not strict:
return obj.__str__()

Here is an example:

from pymatgen.core import Structure
from monty.json import jsanitize
struct = Structure(
    lattice=[[0, 2.13, 2.13], [2.13, 0, 2.13], [2.13, 2.13, 0]],
    species=["Mg", "O"],
    coords=[[0, 0, 0], [0.5, 0.5, 0.5]],
)
jsanitize(struct)
'Full Formula (Mg1 O1)\nReduced Formula: MgO\nabc   :   3.012275   3.012275   3.012275\nangles:  60.000000  60.000000  60.000000\nSites (2)\n  #  SP      a    b    c\n---  ----  ---  ---  ---\n
  0  Mg    0    0    0\n  1  O     0.5  0.5  0.5'

If the desire is to maintain backwards-compatibility, one option is to create an allow_msonable kwarg and introduce the following block of code before the conversion to str:

if allow_msonable and isinstance(obj, MSONable):
    return obj.as_dict()

However, if this is considered to be a bug (unclear based on the description), then the allow_msonable kwarg isn't needed.

@shyuep, what are your thoughts on having jsanitize return MSONable objects via .as_dict() rather than str by default? Was there a specific reason why this was avoided in the current implementation?

I think having a recurse_msonable would be useful. I think the reason why str is the default is to avoid a potentially expensive expansion of MSONAble objects when it is not needed.

Closed in #350.