That's fantastic. But can we move this to the development list, to continue the discussion...
On 11 November 2010 11:28, Andrew Dalke <da...@dalkescientific.com> wrote: > On 09/11/2010 13:53, Andrew Dalke wrote: >> I am also willing to write tests which cover these. > > On Nov 10, 2010, at 11:22 AM, Chris Morley wrote: >> Even better! > > I spent a lot of time on trains yesterday, and test cases were > the right mental level for me. > > Here's what I developed: > > http://dalkescientific.com/test_python.py > > They are meant so that people have a way to figure out how > to call different functions, and they are meant to be fast, > so they can be used as real unit tests. (The 66 test cases / > ~400 tests) run in about 1.5 seconds on my laptop.) > > > I don't know how you all want that hooked into the existing test > framework, but I figured it was best to start with test cases. > > It also looks like I should get commit privs to OpenBabel. > > > I noticed a number of minor problems when developing the tests. > These are all marked with an "XXX" in the above code, although > there are a few places marked with XXX which are either TODOs > or places where I didn't understand the result when I wrote it > but I believe I do now. > > 1) If I do something like > > mol = parse_smiles("CCO") > mol.SetTitle("#1") > conv.WriteFile(mol, tempdir("blah.sdf")) > > I get the message > > Warning in WriteMolecule No 2D or 3D coordinates exist. > Any stereochemical information will be lost. To generate > 2D or 3D coordinates use --gen2D or --gen3d. > > Since I'm not (directly) calling WriteMolecule and since my > test cases don't support the --gen2D or --gen3d flags, this > warning isn't that helpful in a library function. > > 2) ob.OBFingerprint.List("fingerprints") > > This writes to an output stream, and I can't figure out > how to generate a string-based output stream in Python. > However, this is not that important because there are > other ways to access the same data. It does suggest that > List() is not a useful library function. > > 3) Why does the fingerprint GetBit require a fingerprinter instance? > That is, I have to go through a bound method > > x = ob.OBFingerprint.FindFingerprint("FP2") > self.assertEquals(x.GetBit(v, 0), True) > > instead of using a static/global function > > self.assertEquals(ob.OBFingerprint.GetBit(v, 0), True) > > 4) Here's a test case I didn't expect. It seems that GetUMapList > interacts with the results of GetMapList. > > def test_basic_match_behavior_which_I_did_not_expect(self): > mol = parse_smiles("c1ccccc1O") > pat = parse_smarts("c1ccccc1") > pat.Match(mol) > self.assertEquals(pat.NumMatches(), 12) > results = pat.GetUMapList() > self.assertEquals(pat.NumMatches(), 1) > results = pat.GetMapList() > # I really expected these to be 12. > # It appears the UMapList does an in-place trim. > # XXX Is that the right/expected behavior? > self.assertEquals(pat.NumMatches(), 1) > self.assertEquals(len(results), 1) > > pat.Match(mol) > # Here they are 12 > results = pat.GetMapList() > self.assertEquals(pat.NumMatches(), 12) > results = pat.GetUMapList() > self.assertEquals(pat.NumMatches(), 1) > self.assertEquals(len(results), 1) > > > 5) The BeginMList/EndMList seems broken in Python. That is, > I couldn't figure how how to use them. > > 6) I included tests for iterating over atoms and bonds in > a molecule, and for atoms and bonds connected to an atom. I > expected I could also iterate over the atoms in a bond, but > there is no OBBondAtomIter . While not important, it appeals > to my sense of symmetry and it's very simple to write. > > 7) Why can't I do mol.AddBond(C, N, 3) where C and N are atoms? > Instead, I need to do mol.AddBond(C.GetIdx(), N.GetIdx(), 3) > > 8) Why are there two ways to add an atom? These are: > atom = mol.NewAtom() > -and- > atom = OBAtom() > mol.AddAtom(atom) > > Is one of these forms preferred? > > 9) How does OpenBabel define "implicit hydrogen"? It's different > than my definition, which in > > [NH3].C > > is the explicitly stated implicit h-count in [NH3] and the > inferred H-count in C (which is 4). OpenBabel treats the H3 > as explicit. > > It appears that OpenBabel doesn't have an implicit h-count > field and instead tracks the valence, then calculates the > h-count based on the valence. > > 10) Shouldn't the molecular formula for "[NH4+]" include > the total formal charge? > > 11) The formal results for c1ccccc1O.[NH4+] are very strange > > Take a look at this test > > self.assertEquals(mol.GetSpacedFormula(), "C 6 H 10 N 1 O 1 ") > self.assertEquals(mol.GetSpacedFormula(1), "C 6 H 10 N O ") > > Why is there a double space in "N 1" and "O 1"? Why is there a > space after the last "1"? And just how useful is the "include > implicit hydrogens" flag to generate > > self.assertEquals(mol.GetSpacedFormula(0, ' ', 0), "C 6 H 4 N 1 O 1 > ") > > ? > > 12) I don't think atom.GetCoordinate() is accessible from Python > except through ctypes (Python's foreign function interface) > > 13) Why does GetAtom() start from 1 while GetBond() start from 0? > (Perhaps this is in the documentation somewhere, but I was on a > train without access to the internet and Google) > > mol = parse_smiles("C#N") > C = mol.GetAtom(1) > N = mol.GetAtom(2) > # XXX Why do bonds starts from 0 and not 1 > self.assertEquals(mol.GetBond(1), None) > > 14) The documentation for OBBond.GetNbrAtom needs to warn that if > the atom isn't one of the bond atoms then the GetBeginAtom() will > be returned. (This documentation for GetNbrAtomIdx already > has a warning to that effect.) > > 15) In an OBRing, if there are multiple non-C atoms, which one > is the root? And since ring.GetRootAtom() returns an integer, > shouldn't it be named GetRootAtomIdx()? > > 16) Why does GetAngle() return degrees instead of radians? I see > the warning to that effect, but it was rather unexpected to see. > > 17) If you have something like "CNOS" then C.IsConnected(C) is > true. I did not expect that. This is because IsConnected checks > to see if the target is any of the atoms of either side of the > bonds that are connected to C. And of course, C is always one > of the atoms connected to a bond of C. > > This has a downstream consequence in that > > C.IsOneFour(C) == True > > If I read the code right, the C in "CN" will also be in a > 1-4 relationship with itself. > > 18) There are some minor problems with the spectrophoretest.cpp > test01. (I didn't review all of the tests.) > > In > > OB_REQUIRE( (r[13] > 3.470) && (r[13] < 3.472) ); > OB_REQUIRE( (r[14] > 6.698) && (r[14] < 6.800) ); > OB_REQUIRE( (r[15] > 9.485) && (r[15] < 9.487) ); > > the line with r[14] should have 6.700 as the upper bound. > > and > OB_REQUIRE( (r[18] > 4.899) && (r[18] < 4.902) ); > should be slightly smaller in the upper bound > OB_REQUIRE( (r[18] > 4.899) && (r[18] < 4.901) ); > > > 19) I see that MMFF94's ValidateGradients() dumps output > to stdout. The other ValidateGradients I tested (gaff and > ghemical) do not do that, and the output probably isn't > appropriate for a library. > > Then again, the API docs say that it's meant for debugging. > Should the debugging API be published for general use? > > 20) OBAtom.HtoMethtyl() can also dump code to stderr > > v3: < -0.28, 0, 0 > v4: < -0.28, 0, 0 > > > This is quite clearly a bit of debugging code which > wasn't removed: > > cerr << "v3: " << v3 << " v4: " << v4 << endl; > > > Cheers! > > > Andrew > da...@dalkescientific.com > > > > ------------------------------------------------------------------------------ > Centralized Desktop Delivery: Dell and VMware Reference Architecture > Simplifying enterprise desktop deployment and management using > Dell EqualLogic storage and VMware View: A highly scalable, end-to-end > client virtualization framework. Read more! > http://p.sf.net/sfu/dell-eql-dev2dev > _______________________________________________ > OpenBabel-discuss mailing list > OpenBabel-discuss@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openbabel-discuss > ------------------------------------------------------------------------------ Centralized Desktop Delivery: Dell and VMware Reference Architecture Simplifying enterprise desktop deployment and management using Dell EqualLogic storage and VMware View: A highly scalable, end-to-end client virtualization framework. Read more! http://p.sf.net/sfu/dell-eql-dev2dev _______________________________________________ OpenBabel-discuss mailing list OpenBabel-discuss@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openbabel-discuss
