Menu
▾
▴
[Cdk-devel] [ cdk-Patches-2982715 ] new Bond() has two atoms??
|
From: SourceForge.net <no...@so...> - 2010-07-27 18:05:04
|
Patches item #2982715, was opened at 2010-04-06 17:43 Message generated for change (Settings changed) made by egonw You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2982715&group_id=20024 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: cdk-1.2.x >Group: Accepted Status: Open Resolution: None Priority: 5 Private: No Submitted By: Egon Willighagen (egonw) Assigned to: Nobody/Anonymous (nobody) Summary: new Bond() has two atoms?? Initial Comment: OK, the method is likely very rarely used, but I just ran into this bit of unit testing: @Test public void testBond() { IBond bond = new Bond(); Assert.assertEquals(2, bond.getAtomCount()); Assert.assertNull(bond.getAtom(0)); Assert.assertNull(bond.getAtom(1)); Assert.assertNull(bond.getOrder()); Assert.assertEquals(CDKConstants.STEREO_BOND_NONE, bond.getStereo()); } I'm pretty sure this thing does not have two atoms. I looked up the commit that made the assertion that this bond has two atoms, and traced is back to commit 35e8666d9573f3e19684c13cf6ab2c42471c28ed, which made this change: - org.openscience.cdk.interfaces.IAtom[] atoms = b.getAtoms(); - assertEquals(2, atoms.length); - assertEquals(atomsToAdd[0], atoms[0]); - assertEquals(atomsToAdd[1], atoms[1]); + assertEquals(2, b.getAtomCount()); + assertEquals(atomsToAdd[0], b.getAtom(0)); + assertEquals(atomsToAdd[1], b.getAtom(1)); It asserted 2 before the patch too, but the first assert referred to the length of the array, not the atom count... easy mistake, I guess... ---------------------------------------------------------------------- >Comment By: Egon Willighagen (egonw) Date: 2010-07-27 20:05 Message: Hahaha... that's me miscounting... that's the last patch of the current CDK 1.2.x :) Just start with 0002... ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2010-07-27 16:24 Message: Also, path 001 does not apply on latest cdk-1.,2.x, so I didn't try the others ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2010-07-27 16:22 Message: Well right now, IIRC, you will get an out-of-bounds exception. The value at which this occurs depends on how the bond was constructed. If you start the bond with the atom,atom constructor then the bond will always be a 2-center bond. If you create the bond with the IAtom[] constructor, you can set atos up to how many were specified in the constructor. I can sign of on this ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-07-27 15:42 Message: Good question... not sure... I guess it should dynamically increase the array internally... Thanx Christoph and Rajarshi... shall I commit them myself? Rajarshi, or will you signoff and commit? Or prefer to wait for a bit more feedback by others? ---------------------------------------------------------------------- Comment By: Rajarshi Guha (rajarshi) Date: 2010-07-27 15:34 Message: Overall looks good. I think the new behavior is correct. One question - in setAtom, what is the upper bound of position? What happens if some specifies position = 10? ---------------------------------------------------------------------- Comment By: Christoph Steinbeck (steinbeck) Date: 2010-07-27 12:31 Message: I looked at the patches and I agree that new Bond() should have zero atoms and return that accordingly upon request. I reviewed All of the patches with respect to implementing the changes above and I find them coherent and to the best of my knowledge complete. I suggest to apply those patches as they are. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-07-27 11:13 Message: Besides a bug fix, these patches also define a slightly different behavior (though I think intended): * new Bond() has zero atoms * setAtom() changes the atom count But, since it includes behavioral changes, it needs two reviews. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-07-27 11:08 Message: This bug is also present in cdk-1.2.x. So, I've backported the patch, and will now attached 5 new patches. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-06-17 22:40 Message: These four patches are rebased on top of the current master version. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-06-17 22:29 Message: Created a patch. There was a correlated problem with setAtom(IAtom, int) not updating the atom count, which is fixed by the patch too. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-04-06 20:43 Message: One more patch, to fix the same issue for NNBond. ---------------------------------------------------------------------- Comment By: Egon Willighagen (egonw) Date: 2010-04-06 18:09 Message: Two patches applied. One to fix the unit test. Another that fixes the constructor and setAtom() method. ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=320024&aid=2982715&group_id=20024 |