Add extract mol fragment api by bp-kelley · Pull Request #8811 · rdkit/rdkit

bp-kelley · 2025-09-24T12:55:01Z

Fixes the root cause of #8649

This replaces the pr for #8742

…ew ROMol by creating new graph (rdkit#8742) This adds a new api, `RDKit::MolOps::ExtractMolFragment`, to allow efficient extractions of mol fragments from large mols. Compared to the approach where we delete "unwanted" atoms/bonds from the input mol, this api is faster for small mols (about 2x faster) and at least 3x faster for big mols (was 10x faster for "CCC"*1000).

…gmentApi

…l as a new ROMol by creating new graph (rdkit#8742) (rdkit#8743)" This reverts commit 040bdb6. During testing of using this as a replacement for portions of getTheFrags in getMolFrags, several issues came up regarding how copies should actually work in practice. These are being corrected in a new pr: rdkit#8811

rachelnwalker · 2025-09-24T17:52:31Z

Is it possible to add back some of the tests @whosayn had in the original PR?

Also, I would definitely find this useful in python :)

bp-kelley · 2025-09-24T17:56:33Z

@rachelnwalker thanks for the note, I thought I had. I'll readd them.

bp-kelley · 2025-09-24T17:58:31Z

@rachelnwalker I did, there's a merge conflict though. I'll fix it

bp-kelley · 2025-09-24T18:51:17Z

I added back the tests with one change, in commitBatch we only remove atoms from stereogroups, not the stereogroups themselves so I use this behavior. And yes, python/swig all that good stuff should be implemented.

…

On Wed, Sep 24, 2025 at 1:52 PM Rachel Walker ***@***.***> wrote: *rachelnwalker* left a comment (rdkit/rdkit#8811) <#8811 (comment)> Is it possible to add back some of the tests @whosayn <https://github.com/whosayn> had in the original PR? Also, I would definitely find this useful in python :) — Reply to this email directly, view it on GitHub <#8811 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACLOFIBLEEJSMXXS4J54YUD3ULK7LAVCNFSM6AAAAACHL6XYOOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGMZQGA2DCOJUHE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

…l as a new ROMol by creating new graph (#8742) (#8743)" (#8814) This reverts commit 040bdb6. During testing of using this as a replacement for portions of getTheFrags in getMolFrags, several issues came up regarding how copies should actually work in practice. These are being corrected in a new pr: #8811 Co-authored-by: Brian Kelley <bkelley@glysade.com>

greglandrum · 2025-09-25T09:17:39Z

@bp-kelley can you please push an empty commit here so that the CI builds run again?

whosayn

didn't look too closely at the implementation details in this first pass, so my comments will reflect that. my main concern with these changes is the lack of unit tests for the SubsetOptions and SubsetMethod::BOND configs. @bp-kelley is that coming in a subsequent diff?

Code/GraphMol/Subset.h

Code/GraphMol/Subset.cpp

Code/GraphMol/MolOps.cpp

whosayn · 2025-09-29T23:32:37Z

Code/GraphMol/MolOps.cpp

-          }
-        }
+	SubsetOptions opts;
+	opts.copyCoordinates = copyConformers;


nit: consider designated initializers like SubsetOptions opts{.copyCoorsinates = copyConformers, ...};

I'll make these changes when I add the python wrapper

Code/GraphMol/MolOps.cpp

Code/GraphMol/catch_molops.cpp

bp-kelley · 2025-10-02T12:27:49Z

@whosayn @greglandrum I'll tackle some of the review comments and I sorted the ubuntu error with the if statement.

I would like a mechanism to get mapped bonds as well as atoms. For instance, just because the endpoints of a bond are mapped doesn't mean that the bond is. This means to either keep SubsetInfo and probably make the following public as the other functions are helper functions around this.

copyMolSubset(RWMol &mol, SubsetInfo &info, const SubsetOptions &options)

Thoughts?

Once this is in place I'll do the python API.

Then I can show you how to start investigating the weird stuff.

greglandrum · 2025-10-23T09:11:04Z

@whosayn @greglandrum I'll tackle some of the review comments and I sorted the ubuntu error with the if statement.

I would like a mechanism to get mapped bonds as well as atoms. For instance, just because the endpoints of a bond are mapped doesn't mean that the bond is. This means to either keep SubsetInfo and probably make the following public as the other functions are helper functions around this.

I can definitely see that being able to track where bonds go would be useful.

Note: Subset.h SubsetMethod needed to change to an enum class because of a conflict in the java wrappers

bp-kelley · 2025-10-27T15:04:33Z

@whosayn @rachelnwalker @greglandrum I think this is finally ready. I think I have handled all critiques and made the Java and python wrappers.

greglandrum

I haven't finished with Subset.cpp. I'll finish it after the suggested changes have been made

greglandrum · 2025-11-19T16:06:19Z

Code/GraphMol/Subset.h

+#define RD_SUBSET_H
+
+#include <RDGeneral/export.h>
+#include <RDGeneral/BetterEnums.h>


you aren't using this here. Probably don't need to include it

greglandrum · 2025-11-19T16:13:05Z

Code/GraphMol/Subset.h

+//! Subsetting Options for copyMolSubset
+/*
+ * These control what is copied over from the original molecule
+ *     \param sanitize - perform sanitization automatically on the subset


\param is for arguments to a function.
To document data members of a struct, we add the docs directly after declaring the member.
As an example, take a look at the docs of the options in FileParsers.h

Should be resolved

greglandrum · 2025-11-19T16:14:42Z

Code/GraphMol/MolOps.cpp

-            frag->addConformer(conf);
-          }
-        }
+	SubsetOptions opts {


Just noticing this here. I'm sure it applies elsewhere:
Please run clang-format over all of the code in this PR

I'll run clang-format last to help with the PR.

greglandrum · 2025-11-19T16:20:00Z

Code/GraphMol/Wrap/test_subset.py

@@ -0,0 +1,96 @@
+#
+#  Copyright (C) 2003-2021  Greg Landrum and other RDKit contributors


might as well fix this

Code/GraphMol/Wrap/test_subset.py

greglandrum · 2025-11-20T04:16:07Z

Code/GraphMol/Subset.cpp

+    static constinit bool updateLabel = false;
+    static constinit bool takeOwnership = true;
+    atomMapping[ref_atom->getIdx()] = extracted_mol.addAtom(


Curiosity: why are these constinit and not constexpr?

Some of these (and the snake case) came from the original PR, I'll have to ask @whosayn, but constexpr or just const seem to be the right choice here.

greglandrum · 2025-11-20T04:19:47Z

Code/GraphMol/Subset.cpp

+    // we need to update rings now
+    if(reference_mol.getRingInfo()->isInitialized()) {
+      extracted_mol.getRingInfo()->reset();
+    }
+  }


Since we create the molecule just before the call to this function, we're never going to need to do this. Still, I guess it doesn't hurt to include it.

However, I don't think we need to do this for every bond that we add. Can't we move it outside of the loop and do:

Suggested change

// we need to update rings now

if(reference_mol.getRingInfo()->isInitialized()) {

extracted_mol.getRingInfo()->reset();

}

}

// we need to update rings now

if(selectedBonds.any() && reference_mol.getRingInfo()->isInitialized()) {

extracted_mol.getRingInfo()->reset();

}

}

I seem to remember needing this, but you are right about the location.

You didn't move it. Was that intentional?

greglandrum · 2025-11-20T04:22:35Z

Code/GraphMol/Subset.cpp

+  }
+}
+
+[[nodiscard]] static bool is_selected_sgroup(


Why does this need to be [[nodiscard]]?

Also, it's not in the public API, so I won't insist, but we don't use underscores in function/variable names

I think it's a safety thing, if the return value isn't used, it is a warning.

I understand what [[nodiscard]] does, but I think it only makes sense to use it when there's a reason that you shouldn't ignore the return value of the function (like when it's passing back a pointer that you own).
I don't think that's the case here.

The compiler might helpfully optimise it out and you’re relying on a side effect of the function? That might not be the case here but it’s why I’ve used it on the past.

The compiler might helpfully optimise it out and you’re relying on a side effect of the function? That might not be the case here but it’s why I’ve used it on the past.

Definitely could happen, but there are no side-effects here.

Code/GraphMol/Subset.cpp

greglandrum · 2025-11-20T04:28:22Z

Code/GraphMol/Subset.cpp

+  }
+}
+
+static void copySelectedAtomsAndBonds(::RDKit::RWMol &extracted_mol,


Since we're inside the RDKit namespace, I don't think it needs to be used anywhere in this code.

Removed the qualifiers

Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

…ests.java Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

greglandrum · 2025-11-27T12:56:11Z

@bp-kelley let me know when you think this is ready to be reviewed again

bp-kelley · 2025-11-27T17:25:28Z

I’ll be able to make the changes this weekend focusing on the non testing code first. I expect some of the stylistic issues were just not addressed in the original pr that I moved over. I’ll do a style pass and then clang format

bp-kelley · 2025-12-03T20:52:48Z

@greglandrum should be up for a second look now

greglandrum

I tagged a couple of comments from earlier reviews that still have no replies/fixes.
There are others

Please at least look at those

greglandrum · 2025-12-04T06:01:11Z

Code/GraphMol/Wrap/test_subset.py

+
+def check(info, selection):
+  for i,v in enumerate(selection):
+    if v: assert i in info


just a reminder that there are still two comments in this file

greglandrum · 2025-12-04T06:02:10Z

Code/GraphMol/catch_molops.cpp

+  CHECK(extracted_atoms == expected_atoms);
+}
+
+// This test makes sure we correctly extract atoms


greglandrum · 2025-12-04T06:03:03Z

Code/GraphMol/Subset.cpp

+  copySelectedAtomsAndBonds(*extracted_mol, mol, selection_info, options);
+  copySelectedSubstanceGroups(*extracted_mol, mol, selection_info, options);
+  copySelectedStereoGroups(*extracted_mol, mol, selection_info);
+  if(options.copyCoordinates)


please remove all of the one liners

@greglandrum I believe these are all finished.

greglandrum

LGTM

* Create a function to extract some specified atoms from a ROMol as a new ROMol by creating new graph (#8742) This adds a new api, `RDKit::MolOps::ExtractMolFragment`, to allow efficient extractions of mol fragments from large mols. Compared to the approach where we delete "unwanted" atoms/bonds from the input mol, this api is faster for small mols (about 2x faster) and at least 3x faster for big mols (was 10x faster for "CCC"*1000). * clang-format * review comments * cleanup * Consolidate copying subsets of molecules * Readd missing tests * Update comment to restart build * Remove missing test * Remove debugging comment, fix warnings * Fix warnings on gcc11 * Add docs * Make vector<bool> dynamic_bitset<> * Update copyright * Add swig wrappers * Use new designated constructor API * Fix windows builds * Change enum values from unsigned int to integer * Fix unsigned int variable * Update Code/GraphMol/Wrap/test_subset.py Co-authored-by: Greg Landrum <greg.landrum@gmail.com> * Update Code/GraphMol/Subset.cpp Co-authored-by: Greg Landrum <greg.landrum@gmail.com> * Update Code/JavaWrappers/gmwrapper/src-test/org/RDKit/ChemTransformsTests.java Co-authored-by: Greg Landrum <greg.landrum@gmail.com> * Reponse to review * Fix documentation * Remove comments * Remove unnecessary comments * Fix one liners * Change assertion to be clearer (and not one-liners) * Run clang-format --------- Co-authored-by: Your Name <you@example.com> Co-authored-by: Hussein Faara <hussein.faara@schrodinger.com> Co-authored-by: Brian Kelley <bkelley@glysade.com> Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

Your Name and others added 6 commits August 29, 2025 13:26

clang-format

1ecac4f

review comments

6036303

Merge remote-tracking branch 'origin/master' into pr/addExtractMolFra…

8c2c6df

…gmentApi

cleanup

668cc4a

Consolidate copying subsets of molecules

e2c5e0b

bp-kelley mentioned this pull request Sep 24, 2025

Create a function to extract some specified atoms from a ROMol as a new ROMol by creating new graph (#8742) #8743

Merged

bp-kelley mentioned this pull request Sep 24, 2025

Revert "Create a function to extract some specified atoms from a ROMo… #8814

Merged

Readd missing tests

ea8d38b

bp-kelley and others added 3 commits September 25, 2025 08:48

Update comment to restart build

7113f3e

Remove missing test

ca6523b

Remove debugging comment, fix warnings

39a65d3

whosayn reviewed Sep 29, 2025

View reviewed changes

Fix warnings on gcc11

c0a54a7

bp-kelley added 7 commits October 24, 2025 14:54

Add docs

6f2bdb1

Make vector<bool> dynamic_bitset<>

98b950d

Update copyright

a2372d8

Add swig wrappers

f28de04

Use new designated constructor API

a705cee

Merge branch 'master' into pr/addExtractMolFragmentApi

2edd2b0

Note: Subset.h SubsetMethod needed to change to an enum class because of a conflict in the java wrappers

Fix windows builds

2c9af85

bp-kelley and others added 2 commits October 26, 2025 14:55

Change enum values from unsigned int to integer

c3b1f93

Fix unsigned int variable

4ce50d4

greglandrum self-assigned this Nov 19, 2025

greglandrum requested changes Nov 20, 2025

View reviewed changes

bp-kelley and others added 3 commits November 25, 2025 16:16

Update Code/GraphMol/Wrap/test_subset.py

605d474

Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

Update Code/GraphMol/Subset.cpp

5d8a7ca

Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

Update Code/JavaWrappers/gmwrapper/src-test/org/RDKit/ChemTransformsT…

3d83551

…ests.java Co-authored-by: Greg Landrum <greg.landrum@gmail.com>

bp-kelley added 3 commits November 30, 2025 15:35

Reponse to review

dbeb0d1

Merge branch 'master' into pr/addExtractMolFragmentApi

c394136

Fix documentation

11e2f05

greglandrum requested changes Dec 4, 2025

View reviewed changes

bp-kelley added 5 commits December 4, 2025 15:46

Remove comments

1209822

Remove unnecessary comments

f2bfa80

Fix one liners

fce25b9

Change assertion to be clearer (and not one-liners)

75527c6

Run clang-format

d0eccdc

greglandrum approved these changes Dec 9, 2025

View reviewed changes

greglandrum changed the title ~~Pr/add extract mol fragment api~~ Add extract mol fragment api Dec 9, 2025

greglandrum mentioned this pull request Dec 9, 2025

segfault in RWMol::batchRemoveAtoms() #8649

Closed

greglandrum added the enhancement label Dec 9, 2025

greglandrum added this to the 2025_09_4 milestone Dec 9, 2025

greglandrum merged commit 70540c2 into rdkit:master Dec 9, 2025
12 checks passed

greglandrum mentioned this pull request Dec 12, 2025

Stop writing so many atom properties to cxsmiles #9002

Merged

mcs07 mentioned this pull request Feb 4, 2026

CopyMolSubset silently rewires bonds when bondIndices reference atoms not in atomIndices #9084

Closed

		@@ -0,0 +1,96 @@
		#
		# Copyright (C) 2003-2021 Greg Landrum and other RDKit contributors

Conversation

bp-kelley commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rachelnwalker commented Sep 24, 2025

Uh oh!

bp-kelley commented Sep 24, 2025

Uh oh!

bp-kelley commented Sep 24, 2025

Uh oh!

bp-kelley commented Sep 24, 2025 via email

Uh oh!

greglandrum commented Sep 25, 2025

Uh oh!

whosayn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bp-kelley commented Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greglandrum commented Oct 23, 2025

Uh oh!

bp-kelley commented Oct 27, 2025

Uh oh!

greglandrum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bp-kelley commented Sep 24, 2025 •

edited

Loading

bp-kelley commented Oct 2, 2025 •

edited

Loading

bp-kelley Dec 4, 2025 •

edited

Loading