Yes and it's correct, your code is adding bonds to atoms which don't exist
yet! You need to add the atoms of the bond before the bond - 0..k not k ..
n.
As an aside the Mappings API has this method
already (toSubstructureStream()) if you're coming via CDK substructure
Pattern (although it doesn't do the radials/lone pairs... possible but I
never found a use to have them explicitly like that).
In general this code is an inefficient way to build the container... I
think it's O(N^3) - but perhaps only O(N^2) with AtomContainer2 :-). Much
better to add all the atoms, record these in a set/map, then loop all the
bonds. Optionally for best performance you should do this "resync"
operation where you map the source => target AtomRef. Maybe we should add
this to the ACmanipulator, I thought there was something similar
already though.
John
public static IAtomContainer extractSubstructure(IAtomContainer
source, List<IAtom> atoms) {
IAtomContainer output = source.getBuilder().newAtomContainer();
Map<IAtom,IAtom> remap = new HashMap<>();
for (IAtom atom : atoms) {
output.addAtom(atom);
source.getConnectedLonePairsList(atom).forEach(output::addLonePair);
source.getConnectedSingleElectronsList(atom).forEach(output::addSingleElectron);
// resync: get the AtomRef in the context of the new container. This
// presumes atoms gets added at last position which is currently
// always the case
remap.put(atom, output.getAtom(atom.getAtomicNumber() - 1));
}
for (IBond bond : source.bonds()) {
IAtom beg = remap.get(bond.getBegin());
IAtom end = remap.get(bond.getEnd());
if (beg != null && end != null) {
output.addBond(bond);
// or the more efficient but you get a "NEW" bond so may
need to some fudging with
// setting aromatcity/ring membership etc, however if
you're selecting a substructure
// you MUST recalculate these anyways.
// output.addBond(beg.getIndex(), end.getIndex(),
bond.getOrder(), bond.getStereo());
}
}
return output;
}
On Mon, 5 Sept 2022 at 11:10, Uli Fechner <[email protected]> wrote:
>
> Hi,
>
> I get a NoSuchAtomException when executing the following code (that I
> inherited):
>
> public static IAtomContainer extractSubstructure(IAtomContainer source,
> List<IAtom> atoms) {
> IAtomContainer output =
> SilentChemObjectBuilder.getInstance().newAtomContainer();
> int k = 0;
> for (IAtom atom : atoms) {
> output.addAtom(atom);
> source.getConnectedLonePairsList(atom).forEach(lp ->
> output.addLonePair(lp));
> source.getConnectedSingleElectronsList(atom).forEach(se ->
> output.addSingleElectron(se));
> k++;
> for (int i = k; i < atoms.size(); i++) {
> IBond bond = source.getBond(atom, atoms.get(i));
> if (bond != null) {
> output.addBond(bond);
> }
> }
> }
> return output;
> }
>
> The stack trace starts at the line with
> output.addBond(bond);
>
> and continues with the following lines:
>
> org.openscience.cdk.exception.NoSuchAtomException: Atom is not a member of
> this AtomContainer
> at
> app//org.openscience.cdk.silent.AtomContainer2.getAtomRef(AtomContainer2.java:185)
> at
> app//org.openscience.cdk.silent.AtomContainer2.newBondRef(AtomContainer2.java:221)
> at
> app//org.openscience.cdk.silent.AtomContainer2.addBond(AtomContainer2.java:908)
>
> The exception *isn't* thrown if I replace the instantiation of the
> IAtomContainer output with
> IAtomContainer output = new AtomContainer();
> I understand that the exception has something to do with the way
> AtomContainer2 is implemented, but I don't know how to switch to the
> AtomContainer2 implementation and still retain the intent of the code -
> which is to only add atoms, their connected LPs, their connected single
> electrons and their connected bonds, but not connected atoms - to the
> returned subgraph. Best
> Uli
>
>
> _______________________________________________
> Cdk-user mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/cdk-user
>
_______________________________________________
Cdk-user mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/cdk-user
