I did contemplate making the addAtom() return the new "ref" the container has... however I think this is technically an API breakage (void => IAtom return type). This would not have been in prior Java versions but at some point they added the return type to the signature. It's kind of a minor breakage, basically providing the downstream gets recompiled byte code it's OK.
On Mon, 5 Sept 2022 at 14:59, John Mayfield <john.wilkinson...@gmail.com> wrote: > Yup! Sorry just drafting stuff out :-) Made the same mistake in the PR > which used predicates: > https://github.com/cdk/cdk/pull/889/commits/a160def65f79218a410c8fa4fe6ece5e2ed40dde > > > On Mon, 5 Sept 2022 at 14:47, Daniel Katzel <dkat...@gmail.com> wrote: > >> Surely that line output.getAtom( atom.getAtomicNumber() -1 ) >> >> Was meant to use output.getAtomCount() -1 >> >> To get the last atom added? >> >> On Mon, Sep 5, 2022, 6:44 AM John Mayfield <john.wilkinson...@gmail.com> >> wrote: >> >>> 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 <u...@pending.ai> 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 >>>> Cdk-user@lists.sourceforge.net >>>> https://lists.sourceforge.net/lists/listinfo/cdk-user >>>> >>> _______________________________________________ >>> Cdk-user mailing list >>> Cdk-user@lists.sourceforge.net >>> https://lists.sourceforge.net/lists/listinfo/cdk-user >>> >>
_______________________________________________ Cdk-user mailing list Cdk-user@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/cdk-user
