New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add in place and multithread support for more of the MolStandardize code #6970

Merged

greglandrum merged 8 commits into rdkit:master from greglandrum:feat/mt_fragmentchooser

Dec 12, 2023

Member

greglandrum commented Dec 8, 2023

This PR is primarily aimed at getting the various parent operations working in place and to provide multi-threaded versions of them.

greglandrum added 5 commits

December 7, 2023 08:02


          add inPlace version of the LargestFragmentChooser

9173f5c


          more inplace support in standardization

multithreaded inplace support

still needs more looking at pre-PR


          in place tautomer canonicalization and tautomer parent

ff0862e


          warning removal

bf86f88


          add python wrappers for that

6c19c81

greglandrum added the enhancement label

greglandrum added this to the 2023_09_4 milestone

greglandrum added 2 commits

December 8, 2023 11:19


          unrelated: make boost::stacktrace work again

787e759

should have noticed this a while ago


          sanitize the result of fragmentInPlace to be consistent with previous…

b1ef899

… behavior

greglandrum requested a review from ptosco

December 8, 2023 15:36

ptosco approved these changes

View reviewed changes

Contributor

ptosco left a comment

I left in a few comments.

Code/GraphMol/MolStandardize/Fragment.cpp Outdated

-                  double weight = Descriptors::calcExactMW(*frag);
-                  if (l.Fragment != nullptr &&
-                      (!this->useAtomCount || numatoms == l.NumAtoms) &&
+                  double weight = 0;

Contributor

ptosco Dec 12, 2023

I know this is not required, but I think it looks tidier:

Suggested change

      
                double weight = 0;
          
                double weight = 0.0;

Code/GraphMol/MolStandardize/Fragment.cpp

+                    const auto atom = mol.getAtomWithIdx(idx);
+                    // it's not important to be perfect here
+                    weight += 100 * atom->getAtomicNum() + atom->getIsotope() -
+                              atom->getFormalCharge() * .1;

Contributor

ptosco Dec 12, 2023

Making weight depend on formal charge looks a bit of a hack, and could lead to unintentional ties (10 formal charges count like one neutron). Wouldn't it be better to leave formal charge out of the weight computation and rather add it as an additional compare criterion?

Member Author

greglandrum Dec 12, 2023

this is clearly a heuristic (sounds nicer than "hack") and it doesn't have to be exactly correct, just reproducible.

Plus, how often are you going to have a molecule with two fragments which have the same number of atoms and the same atoms but which differ by one neutron and a net formal charge difference of 10?

I think it's a theoretical risk. :-)

Contributor

ptosco Dec 12, 2023

It is a hack :-) but I am fine with it.

Code/GraphMol/MolStandardize/MolStandardize.cpp Outdated

+              void tautomerParentInPlace(std::vector<RWMol *> &mols, int numThreads,
+                                         const CleanupParameters &params,
+                                         bool skip_standardize) {
+                auto sfunc = [&](RWMol &m, const CleanupParameters &ps) {

Contributor

ptosco Dec 12, 2023

You actually only need to capture skip_standardize.

Code/GraphMol/MolStandardize/MolStandardize.cpp Outdated

+              void fragmentParentInPlace(std::vector<RWMol *> &mols, int numThreads,
+                                         const CleanupParameters &params,
+                                         bool skip_standardize) {
+                auto sfunc = [&](RWMol &m, const CleanupParameters &ps) {

Contributor

ptosco Dec 12, 2023

You actually only need to capture skip_standardize.

Code/GraphMol/MolStandardize/MolStandardize.cpp Outdated

+              void stereoParentInPlace(std::vector<RWMol *> &mols, int numThreads,
+                                       const CleanupParameters &params,
+                                       bool skip_standardize) {
+                auto sfunc = [&](RWMol &m, const CleanupParameters &ps) {

Contributor

ptosco Dec 12, 2023

You actually only need to capture skip_standardize.

Member Author

greglandrum Dec 12, 2023

I think your keyboard got stuck! ;-)

Code/GraphMol/MolStandardize/MolStandardize.cpp Outdated

                 std::unique_ptr<Reionizer> reionizer{reionizerFromParams(params)};
                 reionizer->reionizeInPlace(mol);
               }
-              void reionizeInPlace(std::vector<RWMol *> &mols,int numThreads,
+              void reionizeInPlace(std::vector<RWMol *> &mols, int numThreads,
                                    const CleanupParameters &params) {
                 std::unique_ptr<Reionizer> reionizer{reionizerFromParams(params)};
                 auto sfunc = [&](RWMol &m, const CleanupParameters &) {

Contributor

ptosco Dec 12, 2023

Here's there is no need to capture anything.

Member Author

greglandrum Dec 12, 2023

actually, it needs to capture reionizer, but only that

Code/GraphMol/MolStandardize/MolStandardize.cpp Outdated

                 remover->removeInPlace(mol);
               }
-              void removeFragmentsInPlace(std::vector<RWMol *> &mols,int numThreads,
+              void removeFragmentsInPlace(std::vector<RWMol *> &mols, int numThreads,
                                           const CleanupParameters &params) {
                 std::unique_ptr<FragmentRemover> remover{fragmentRemoverFromParams(params)};
                 auto sfunc = [&](RWMol &m, const CleanupParameters &) {

Contributor

ptosco Dec 12, 2023

Here's there is no need to capture anything.

Code/GraphMol/MolStandardize/Wrap/rdMolStandardize.cpp

+                if (params) {
+                  ps = python::extract<RDKit::MolStandardize::CleanupParameters *>(params);
+                }
+                func(*static_cast<RDKit::RWMol *>(mol), *ps, skip_standardize);

Contributor

ptosco Dec 12, 2023

So one can cast a ROMol* to RWMol* without Bad Consequences? I'll keep that in mind if I do not need a copy.

Member Author

greglandrum Dec 12, 2023

Well, it is officially UB. But in the RDKit the only difference between RWMol and ROMol is that an RWMol has a few more methods. There are no additional data members, so I think that doing that cast is as safe as UB ever can be.

Code/GraphMol/MolStandardize/Wrap/rdMolStandardize.cpp Outdated

+              void mtinPlaceHelper2(python::object pymols, int numThreads,
+                                    python::object params, bool skip_standardize,
+                                    FUNCTYPE func) {
+                const RDKit::MolStandardize::CleanupParameters *ps =

Contributor

ptosco Dec 12, 2023

Suggested change

      
              const RDKit::MolStandardize::CleanupParameters *ps =
          
              const auto ps =

Member Author

greglandrum Dec 12, 2023

this one actually isn't so easy since we plan to reassign ps later. It needs to be const auto *ps

Code/GraphMol/MolStandardize/Wrap/rdMolStandardize.cpp Outdated

+                unsigned int nmols = python::extract<unsigned int>(pymols.attr("__len__")());
+                std::vector<RDKit::RWMol *> mols(nmols);
+                for (auto i = 0u; i < nmols; ++i) {
+                  RDKit::RWMol *mol = static_cast<RDKit::RWMol *>(

Contributor

ptosco Dec 12, 2023

Suggested change

      
                RDKit::RWMol *mol = static_cast<RDKit::RWMol *>(
          
                auto mol = static_cast<RDKit::RWMol *>(


          changes in response to review

1e96b97

greglandrum merged commit c7c9ad3 into rdkit:master

10 checks passed

greglandrum deleted the feat/mt_fragmentchooser branch

December 12, 2023 16:21

greglandrum added a commit that referenced this pull request


          Add in place and multithread support for more of the MolStandardize c…

a978054

…ode (#6970)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment