Dev/reaction enumeration #1111

bp-kelley · 2016-10-18T01:01:27Z

Finally in a mergable state, an enumeration toolkit! See the ipython notebook in the ChemReactions/tutorial directory for more details.

Hopefully it builds on windows :)

bp-kelley · 2016-10-18T01:01:58Z

Let me know if I should just squash all this stuff btw and resubmit.

greglandrum · 2016-10-18T07:28:59Z

I have github setup to squash things on merge, so no worries about that.

I just re-triggered the appveyor build to see if that was a random failure (like, I suspect, the python3.4 failure was). I'm traveling and don't have my windows machine with me, so me looking at that would have to wait for the weekend.

bp-kelley · 2016-10-18T11:21:21Z

There is an infinite iterator bug in python3 anyway, that will take me a while to sort out.

bp-kelley · 2016-10-20T20:38:55Z

@greglandrum - I'm going to add a proper options class to the enumerator and a constructor that takes a serialization string, so expect at least one more update.

I'm also going to allow setting a new enumeration strategy after the fact, I've already been annoyed that it doesn't exist ;)

greglandrum

I'm not done yet, but I wanted to get something submitted in the near term while I finish reading through

greglandrum · 2016-10-21T09:54:12Z

Code/GraphMol/ChemReactions/CMakeLists.txt

@@ -1,21 +1,42 @@
+find_package(Boost 1.39.0 COMPONENTS serialization REQUIRED)
+message("==> ${Boost_SERIALIZATION_LIBRARY}")


Probably don't need this message

greglandrum · 2016-10-21T09:56:35Z

Code/GraphMol/ChemReactions/CMakeLists.txt


 rdkit_test(testReaction testReaction.cpp LINK_LIBRARIES
-ChemReactions FilterCatalog ChemTransforms Descriptors Fingerprints Subgraphs DataStructs Depictor FileParsers SmilesParse SubstructMatch
+${Boost_SERIALIZATION_LIBRARY} ChemReactions ChemTransforms Descriptors Fingerprints Subgraphs DataStructs Depictor FileParsers SmilesParse SubstructMatch


The boost serialization library should probably be at the end of the list of libraries. Otherwise I think we'll get undefined symbols if statically linking on (at least) windows.

greglandrum · 2016-10-21T09:59:56Z

Code/GraphMol/ChemReactions/Enumerate/EnumerateTypes.h

+namespace RDKit {
+//! BBS - Helper typedef for holding buliding blocks for reactions
+//!   holds vectors of reagents for each reactant in a Reaction
+typedef std::vector<MOL_SPTR_VECT> BBS;


Given how generic the names BBS and RGROUPS are, I think they should be in their own namespace. How about RDKit::EnumerationTypes::BBS and RDKit::EnumerationTypes::RGROUPS ?

greglandrum · 2016-10-21T10:04:55Z

Code/GraphMol/ChemReactions/Enumerate/CartesianProduct.h

+
+  operator bool() const { return hasNext(); }
+
+  EnumerationStrategyBase *Clone() const {


An aside for us to try to remember later when we do API cleanups:
This method is called copy() on atoms and bonds.
You've used Clone() here and in the FilterCatalog.
We should probably make that consistent.

I'll deprecate clone in FilterCatalog and switch over to copy.

greglandrum · 2016-10-22T10:20:18Z

Code/GraphMol/ChemReactions/Enumerate/CartesianProduct.h

+    return m_permutation;
+  }
+
+  size_t getPermutationIdx() const { return m_numPermutationsProcessed; }


Another consistency thing to think about (though I won't insist pre-merge, I am pretty certaion to change it post-merge): size_t isn't used anywhere else in the RDKit API (at least it should be). We tend to use unsigned int or one of the boost::uint typedefs when the size is important

uint64_t is more appropriate here since the number of permutations can be really large here. I'll make the changes.

greglandrum · 2016-10-22T10:22:09Z

Code/GraphMol/ChemReactions/Enumerate/CartesianProduct.h

+    }
+  }
+
+  void next(size_t rowToIncrement) {


This method ought to have a PRECONDITION ensuring that rowToIncrement is valid

greglandrum · 2016-10-22T10:26:02Z

Code/GraphMol/ChemReactions/Enumerate/Enumerate.cpp

+  setState(m_initialState);
+}
+
+std::vector<std::vector<std::string> > EnumerateLibraryBase::nextSmiles() {


Most of the rest of the code would take the vector to be returned as a reference parameter. As long as we aren't using C++11, doesn't returning a vector of vectors of strings do rather a lot of copying?

Note to self: I made it this far in the first part of the review.

I believe this is return value optimized optimized by copy elision.

http://en.cppreference.com/w/cpp/language/copy_elision

It can also be a move constructor in c++11. In any-case if there is a copy, it is really low cost for standard returns and will not be a copy in c++11 and 14.

greglandrum · 2016-10-22T10:29:50Z

Code/GraphMol/ChemReactions/Enumerate/EnumerateTypes.h

+//!   building_blocks.push_back( BBS[0][groups[0] );
+//!   building_blocks.push_back( BBS[1][groups[1] );
+//!    rxn.runReactants( building_blocks );
+typedef std::vector<size_t> RGROUPS;


I assume that these are going to be serialized at some point, right?
If you serialize a word-size dependent value like size_t on a 64bit system and then deserialize on a 32bit system, is boost::serialize smart enough to deal with that?
(This is part of my general unhappiness with size_t being used in the public API).

RGRoups aren't serialized, they are just the index into the bb list so they need to be of vector::size_t type or larger. I agree completely that size_t should only be used for vector types and not part of the api. I'll make these uint64_t.

As an aside, I'm using a text archive which is the most portable (if slower) version. There is a portable binary archive, but since most of the data is in the molecule pickle, I don't think it is necessary.

greglandrum · 2016-11-04T05:21:46Z

@bp-kelley : looks like that broke the travis build

get the tests working on windows

greglandrum · 2016-11-05T12:53:44Z

That did it. I think this is ready to merge (and then to tag as "beta" for this release). I'll do that as soon as the travis build pass.

bp-kelley · 2016-11-05T12:55:43Z

I'm going to make sure the tutorial still works, that will take about 10-20 minutes, but we are ready to go.

Where should I put Glare by the way? Contrib?

greglandrum · 2016-11-05T13:26:38Z

yeah, I think we should start with GLARE in contrib. We can move it to somewhere in rdkit.Chem once we have a bit more experience with it?

greglandrum · 2016-11-05T13:27:06Z

Ok, let me know when to merge. I should be around for about another hour

bp-kelley · 2016-11-05T13:32:35Z

@greglandrum - Ready to go!

Hold on, moving Glare...

bp-kelley · 2016-11-05T13:42:02Z

Ok! Thanks for spending the time reviewing, it was quite useful and necessary :)

greglandrum · 2016-11-05T13:42:34Z

Thanks for the code... it's cool stuff.

And I think we'll probably do a bit more tweaking still. :-)

bp-kelley · 2016-11-05T13:53:24Z

@greglandrum I think I already have done some tweaking, I had to stop and pull the trigger at some point :)

greglandrum · 2016-11-05T14:10:09Z

@bp-kelley yeah, but I want to play too! ;-)

Brian Kelley added 17 commits October 14, 2016 11:02

Adds C++ Enumeration Engine to the RDKit

6ea16bb

Adds Sanitization helpers, wrappers and tests

b1d17b4

Clang format

42851a4

Remove unused enumerationStateOnly flag

28d00d8

Fixes docStrings to current API

6723cf7

Adds doc strings

05024cb

Removes RGroupPosition, adds getPosition to EnumerationBase

946eed1

Fixes readability.

be59ddc

Adds EnumerateLibraryBase::reset and getReaction

d3855d0

Added getReagents method to EnumerateLibrary

b87eed3

Make the tests have the same naming

c6a49b5

Need to save the initial state for resetting.

3c1042c

Stupid case-insensitive file systems

c3b5a39

Moves ResetState to EnumerateLibraryBase

c70f60e

Adds removeNonmatchingReagents helper

9c9a785

Renames currentPosition to getPosition

2eae4a3

Adds Enumeration Toolkit tutorial

3a6e2ed

Brian Kelley and others added 4 commits October 18, 2016 12:14

Fixes Python3 serialization and enumerators

36c6e08

Verified to run on python2 and 3

b1a8dc1

Fixes integer issues on windows

38e192b

The number of enumeration should be unsigned.

e54d1d6

Adds deserialization constructor

7f46ead

greglandrum reviewed Oct 22, 2016

View reviewed changes

Brian Kelley added 3 commits October 22, 2016 09:35

Moves boost_serialization to the end

c93342b

Deprecates Clone in favor of copy

4224d3f

Update tests to use copy.copy not Clone

90974dc

Brian Kelley added 3 commits November 3, 2016 19:41

Adds better testing of the even pair sampling

739e5a3

Fixes namespace

a7c72d2

One more try to fix gcc

8e26d64

Brian Kelley and others added 11 commits November 4, 2016 07:28

Enum classes are c++11 and a microsoft extension.

2264203

Fix typo

3ad39ee

Fixes np.median for python3

2855ff3

Fixes atom iterators

d425ba7

Adds virtual tags to derived virtual functions (for clarity)

d1740af

Fixes size comparison issues

950dc97

Adds doc string

345c31f

Small cleanup (has no effect since flags aren’t used)

abaaac4

fixes crash bug on windows

91e68db

get the tests working on windows

bfe8776

Merge pull request #5 from greglandrum/dev/ReactionEnumeration

4b8a2ef

get the tests working on windows

greglandrum approved these changes Nov 5, 2016

View reviewed changes

Updates tutorial

43a5ee2

Adds Glare implementation to Contrib

7694687

greglandrum added the enhancement label Nov 5, 2016

greglandrum added this to the 2016_09_1 milestone Nov 5, 2016

greglandrum merged commit fa89438 into rdkit:master Nov 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev/reaction enumeration #1111

Dev/reaction enumeration #1111

bp-kelley commented Oct 18, 2016

bp-kelley commented Oct 18, 2016

greglandrum commented Oct 18, 2016

bp-kelley commented Oct 18, 2016

bp-kelley commented Oct 20, 2016

greglandrum left a comment

greglandrum Oct 21, 2016

greglandrum Oct 21, 2016

greglandrum Oct 21, 2016

greglandrum Oct 21, 2016

bp-kelley Oct 22, 2016

greglandrum Oct 22, 2016

bp-kelley Oct 22, 2016

greglandrum Oct 22, 2016

greglandrum Oct 22, 2016

greglandrum Oct 22, 2016

bp-kelley Oct 22, 2016

greglandrum Oct 22, 2016

bp-kelley Oct 22, 2016

greglandrum commented Nov 4, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016

greglandrum commented Nov 5, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016 •

edited

bp-kelley commented Nov 5, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016

greglandrum commented Nov 5, 2016

		@@ -1,21 +1,42 @@
		find_package(Boost 1.39.0 COMPONENTS serialization REQUIRED)
		message("==> ${Boost_SERIALIZATION_LIBRARY}")


		operator bool() const { return hasNext(); }

		EnumerationStrategyBase *Clone() const {

Dev/reaction enumeration #1111

Dev/reaction enumeration #1111

Conversation

bp-kelley commented Oct 18, 2016

bp-kelley commented Oct 18, 2016

greglandrum commented Oct 18, 2016

bp-kelley commented Oct 18, 2016

bp-kelley commented Oct 20, 2016

greglandrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglandrum commented Nov 4, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016

greglandrum commented Nov 5, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016 • edited

bp-kelley commented Nov 5, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016

greglandrum commented Nov 5, 2016

bp-kelley commented Nov 5, 2016 •

edited