Review rdkit.Chem.pharm#D modules #1201
Conversation
gedeck
commented
Dec 13, 2016
gedeck
commented
- General cleanup of code
- Extended some of the unittests
- Moved running of doctest into unittest
Task-Url: rdkit#1194 Either wrapped the range expression into a list or made sure that the code is working with a range object.
| @@ -92,7 +95,7 @@ def GetBitDescriptionAsText(self, bitIdx, includeBins=0, fullPage=1): | |||
| a string with the HTML | |||
|
|
|||
| """ | |||
| nPts, combo, scaffold, labels, dMat = self._GetBitSummaryData(bitIdx) | |||
| raise NotImplementedError('Missing implementation') | |||
There was a problem hiding this comment.
nice one. This must have been a leftover from a (much) older version of the code
| @@ -0,0 +1,30 @@ | |||
| # $Id$ | |||
There was a problem hiding this comment.
Please no new SVN droppings.
There was a problem hiding this comment.
Removed all SVN tags from the Pharm2D and Pharm3D folders
| @@ -0,0 +1,30 @@ | |||
| # $Id$ | |||
| # | |||
| # Copyright (C) 2002-2006 greg Landrum and Rational Discovery LLC | |||
There was a problem hiding this comment.
year could be updated, but it's not critical
There was a problem hiding this comment.
I think this is a more general question on how to handle the legal blurb. I understand the situation is more complex due to the history of the code, however what about having a generic statement in each file referring to the license.txt ((C) Rational Discovery LLC, Greg Landrum, and Julie Penzotti) file for all original files (btw. year there is 2006-2015). But then, what to do about modifications?
| @@ -129,8 +131,8 @@ def testOrderBug(self): | |||
| m2 = next(suppl) | |||
| sig1 = Generate.Gen2DFingerprint(m1, self.factory) | |||
| sig2 = Generate.Gen2DFingerprint(m2, self.factory) | |||
| ob1 = set(sig1.GetOnBits()) | |||
| ob2 = set(sig2.GetOnBits()) | |||
| # ob1 = set(sig1.GetOnBits()) | |||
There was a problem hiding this comment.
Might as well just delete these.
| def test1(self): | ||
| """ simple tests | ||
| def test_NotImplemented(self): | ||
| assert LazyGenerator is None, 'Review LazyGenerator unit tests' |
There was a problem hiding this comment.
Should really be using the UnitTest methods (though the rest of the file doesn't currently do so)
There was a problem hiding this comment.
The LazyGenerator code is not working. This assert is there in case someone starts to fix it. Changed it.
| class TestCase(unittest.TestCase): | ||
|
|
||
| def setUp(self): | ||
| self.dataDir = os.path.join(RDConfig.RDCodeDir, 'Chem/Pharm3D/test_data') | ||
| self.fdefBlock = \ | ||
| """DefineFeature HAcceptor1 [N,O;H0] | ||
| self.fdefBlock = """ |
There was a problem hiding this comment.
The parser probably doesn't care, but this is a different string, so it's not just a simple re-fomatting
There was a problem hiding this comment.
You're right. I give it a positive spin though. We also test that the parser is capable of handling extra lines ;-)
| @@ -0,0 +1,37 @@ | |||
| # $Id$ | |||
There was a problem hiding this comment.
please no new subversion droppings in code
| @@ -0,0 +1,37 @@ | |||
| # $Id$ | |||
| # | |||
| # Copyright (C) 2004-2008 greg Landrum and Rational Discovery LLC | |||
There was a problem hiding this comment.
usual thing about the name and date on new copyrights
| @@ -28,8 +25,8 @@ def feq(n1, n2, tol=1e-5): | |||
| class TestCase(unittest.TestCase): | |||
|
|
|||
| def setUp(self): | |||
| self.fdefBlock = \ | |||
| """DefineFeature HAcceptor1 [N,O;H0] | |||
| self.fdefBlock = """ | |||
There was a problem hiding this comment.
same as above: This is now a different string than before, so there should be an actual reason for this change.
There was a problem hiding this comment.
Reason: aversion against \
| @@ -1,6 +1,7 @@ | |||
| tests = [ | |||
| ("python", "EmbedLib.py", {}), | |||
| # ("python", "EmbedLib.py", {}), | |||
There was a problem hiding this comment.
Given that file still contains doctests (and should still contain doctests), they should be run. Please turn this back on.
There was a problem hiding this comment.
UnitTestEmbed runs the doctest, so I remove this line.
