From: Egon W. <ego...@gm...> - 2008-09-26 11:55:22
|
On Thu, Sep 25, 2008 at 12:52 PM, Stefan Kuhn <ste...@eb...> wrote: > Ok, I did the fix once more. It turned out the reason was the hybridisation in > the atom type being set to sp2, which was either not done till now or not > taken into account (note the reason was not the fingerprinting as a such, > since isomorphism check was affected by this as well). OK, it I understand correctly, this applies to this bit of code (TemplateHandler3D, lines 218-219): IAtomContainer ringSystemAnyBondAnyAtom = createAnyAtomAnyBondAtomContainer(ringSystems); BitSet ringSystemFingerprint = new Fingerprinter().getFingerprint(ringSystemAnyBondAnyAtom); And this fix to the first called method: setHybridization(null) Now, that patch does makes sense... because you do not want to query anyBond, with a SP2 bond... Obviously this thing had to be overwritten, because the Fingerprinter takes into account aromaticity! It's really quite stupid (the code, not the author(s)) to try to perceive aromaticity on a AnyAtom-AnyBond graph... instead, a new 'fingerprinter' should have been used that actually applies to a AnyAtom-AnyBond graph... or, if the TemplateHandler3D assumes aromaticity is not important, than make this assumption explicit by any means... And this is really the underlying causes of many instabilities in CDK code: misuse of existing methods and incorrect assumptions. > Sorry for being nasty, but this is a real problem. Tests are there not just > for fun and somebody somewhere must have introduced a change which caused > this. True. Unfortunately, the builder3d code does not come with unit tests... only with macro tests, which are really unsuitable for the application of regression detection/fixing. Egon -- ---- http://chem-bla-ics.blogspot.com/ |