Issue 1998

[jaclark5]

• Tag issue being addressed. Resolves Document that Molecule.from_dict accepts a list of lists but not a numpy array  #1998
• [NA] Add tests
• Update docstrings/documentation, if applicable
• [NA] Lint codebase
• Update changelog

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.11%. Comparing base (9934858) to head (0f3b6a1).
Report is 2 commits behind head on main.

Additional details and impacted files

[jaclark5]

In the doctoring for FrozenMolecules.to_dict I left out reference to cached_properties since it seems like they aren't used.

In my description of the properties output, I described atom_map and left the rest ambiguous. I could make it more specific by specifying that the remainder of the dictionary entries are the output from OEGetSDDataIter and GetPropsAsDict for OpenEye and RDKit respectively.

[mattwthompson]

LGTM % two tiny errors I found (annotating the units as float) which I believe should be updated before merge.

I found a lot of other details that either are out of scope here and don't need fixing or can go either way at your discretion, a.k.a. non-blocking and don't require a re-review unless you wish for one

[mattwthompson]

This dict is empty by default, i.e. not including the atom map, so I'd prefer either not listing it here or being more general about what can go in. IIRC it's a dumping ground for just about anything that can be represented in memory

In [10]: Molecule.from_smiles("CCO").to_dict()['properties']
Out[10]: {}

[jaclark5]

So I made a comment on the PR wondering if I should be more specific since I can see the functions used by the toolkit for the other kwargs. I'll take that as a no :)

I can see that if this is a dumping ground for metadata, it shouldn't be specified, but in the case of atom_map, I might be uploading a molecule with a mapped CMILES and a geometry I got elsewhere... in that case I might need to figure out how to do this manually, which is why I thought it would be useful to explain.

[jaclark5]

In the doctoring of to_smiles it is encouraged for a user to supply an atom_map for this purpose, but doesn't define its format. I think it's important to define it, and I believe this is the place to do it.

[mattwthompson]

Ah totally my mistake, I didn't read the second half of that comment

IMHO

• I have a preference for whatever gets this over the finish line with the lowest friction (I think the original objective of structure of Molecule-generated dictionaries is more than met)
• The added docs/examples you're considering adding are valuable but may be better for different portions of the docs than the docstrings of some serialization methods. I know CMILES is covered in the molecule cookbook but I forget the details

[jaclark5]

I see that it is "verbally" explained in from_smiles I can remove this if you want, or you can save me the commit.

[mattwthompson]

Either is fine with me, tiebreaker going to "do nothing because that's quicker"

(I tagged the unit string as the only blocker, everything else is up to you)