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
