support reading/writing atom props from SD files

[greglandrum]

This still needs documentation in the "RDKit Book", but the basic idea is to allow automatic extraction of atomic property values from SDF data fields.
Here's a sample SDF showing the format:

test-mol
     RDKit  2D

  3  3  0  0  0  0  0  0  0  0999 V2000
    0.8660    0.0000    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0
   -0.4330    0.7500    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0
   -0.4330   -0.7500    0.0000 C   0  0  0  0  0  0  0  0  0  0  0  0
  1  2  1  0
  2  3  1  0
  3  1  1  0
M  END
>  <atom.dprop.PartialCharge>  (1) 
0.008 -0.314 0.008

>  <atom.iprop.NumHeavyNeighbors>  (1) 
2 2 2

>  <atom.prop.AtomLabel>  (1) 
C1 N2 C3

>  <atom.bprop.IsCarbon>  (1) 
1 0 1

>  <atom.prop.PartiallyMissing>  (1) 
one n/a three

>  <atom.iprop.PartiallyMissingInt>  (1) 
[?] 2 2 ?
$$$$

This does not happen by default when processing SDFs; you have to call supp.SetProcessPropertyLists(True) in order to have it go.

[bp-kelley]

I would make this a bit clearer

Atom Property List ... found, require properties for X atoms.  Ignoring it.

[bp-kelley]

Isn't the standard missingValueMarker n/a?

So with this code, one can't write: [n/a] 1 2 since the token can only be one character. Also, what if the form is:

[] 1 2

This would make the missing token "]"

I would take everything between [ and ] for consistency.

[bp-kelley]

I was incorrect about what substr was doing.

[bp-kelley]

Add regression test for atom prop with multiple chars

[n/a] 1 2 [n/a]

[bp-kelley]

We need to make it clear in the spec that "missing values" don't assign properties at all. This will be important in the docs.

[bp-kelley]

I might think about moving the loop through getPropList out of the apply:

for(auto prop:mol.getPropList()) {
  if (prop.find("atom.prop.") == 0 ...)
     applyMolListPropsToAtoms<std::string>(mol, prop, missingValueMarker);
}

This would remove some work (3 extra loops and checks) and perhaps make the code a bit clearer.

[greglandrum]

I actually think this would make the code less flexible.
It would save a bit of work, but this is not going to end up being a measurable fraction of the time required to construct a molecule, so I think that's ok

[bp-kelley]

Any reason for this not to be the default (true)?

[bp-kelley]

Looking at the python api, I feel that exposing the property processing to python outside of the forward mol supplier might not be such a bad idea.

[bp-kelley]

@greglandrum I added my comments, I think a few things need to be resolved, but it mostly looks god.

[bp-kelley]

I think the parser should accept

[n/a] 1 2 n/a

which it currently doesn't (see comments).

We can get rid of some loops in the atom prop handling by removing the loops out of the lowest function.

[bp-kelley]

Add regression test for atom prop with multiple chars

[n/a] 1 2 [n/a]

[bp-kelley]

good catch!