Merge pull request #1228 from rmcgibbo/docstrings

Better docstrings

wrappers/python/src/swig_doxygen/OpenMM.i

@@ -45,7 +45,7 @@ using namespace OpenMM;
 
 %}
 
-%feature("autodoc", "1");
+%feature("autodoc", "0");
 %nodefaultctor;
 
 %include features.i

wrappers/python/src/swig_doxygen/swigInputBuilder.py

@@ -49,8 +49,7 @@ def striphtmltags(s):
     s = s.replace('<i>', '_').replace('</i>', '_')
     s = s.replace('<b>', '*').replace('</b>', '*')
 
-    s = re.sub('\s*(<ul>.*</ul>\s*)', replace_ul_tags, s, flags=re.MULTILINE | re.DOTALL)
-
+    s = re.sub('\s*(<ul>.*?</ul>\s*)', replace_ul_tags, s, flags=re.MULTILINE | re.DOTALL)
     return s
 
 def trimToSingleSpace(text):
@@ -146,6 +145,20 @@ def getClassMethodList(classNode, skipMethods):
     return methodList
 
 
+def docstringTypemap(cpptype):
+    """Translate a C++ type to Python for inclusion in the Python docstrings.
+    This doesn't need to be perfectly accurate -- it's not used for generating
+    the actual swig wrapper code. It's only used for generating the docstrings.
+    """
+    pytype = cpptype
+    if pytype.startswith('const '):
+        pytype = pytype[6:]
+    if pytype.startswith('std::'):
+        pytype = pytype[5:]
+    pytype = pytype.strip('&')
+    return pytype.strip()
+
+
 class SwigInputBuilder:
     def __init__(self,
                  inputDirname,
@@ -568,32 +581,58 @@ class SwigInputBuilder:
             (shortClassName, memberNode,
              shortMethDefinition, methName,
              isConstructors, isDestructor, templateType, templateName ) = items
+
             if self.fOutDocstring:
-                for dNode in findNodes(memberNode, 'detaileddescription'):
-                    dString=""
+                signatureParams = findNodes(memberNode, 'param')
+                assert len(findNodes(memberNode, 'detaileddescription')) == 1
+                dNode = findNodes(memberNode, 'detaileddescription')[0]
+
                 try:
                     description=getText('para', dNode)
                     description.strip()
-                        if description:
-                            dString=description
                 except IndexError:
-                        pass
+                    description = ''
                 params = findNodes(dNode, 'para/parameterlist/parameteritem')
+
+                paramString = ['Parameters', '----------']
+                returnString = ['Returns', '-------']
+
                 if len(params) > 0:
-                        dString="%s\n   Parameters:" % dString
-                    for pNode in params:
-                        argName = getText('parameternamelist/parametername', pNode)
+                    if len(signatureParams) != len(params):
+                        raise ValueError('docstring in %s.%s does not match the signature' % (shortClassName, methName))
+
+                    for pNode, pSignatureNode in zip(params, signatureParams):
+                        parameterNameNode = findNodes(pNode, 'parameternamelist/parametername')[0]
                         argDoc = getText('parameterdescription/para', pNode)
-                        dString="%s\n    - %s %s" % (dString, argName, argDoc)
-                        dString.strip()
+                        argName = getNodeText(parameterNameNode)
+                        argType = docstringTypemap(getText('type', pSignatureNode))
+
+                        isOutput = parameterNameNode.get('direction') == 'out'
+                        if isOutput:
+                            returnString.extend(['%s : %s' % (argName, argType), '    %s' % argDoc])
+                        else:
+                            paramString.extend(['%s : %s' % (argName, argType), '    %s' % argDoc])
+
+
+                returnSection = findNodes(dNode, 'para/simplesect')
+                if len(returnSection) > 0:
+                    returnNode = returnSection[0]
+                    if returnNode.get('kind') == 'return':
+                        argType = getNodeText(findNodes(memberNode, 'type')[0])
+                        argType = docstringTypemap(argType)
+                        returnString.extend([argType, '    %s' % getNodeText(returnNode).strip()])
+
+                dString = '\n'.join(
+                    ([description] + [''] if len(description) > 0 else []) +
+                    (paramString + [''] if len(paramString) > 2 else []) +
+                    (returnString if len(returnString) > 2 else [])).strip()
                 if dString:
-                        dString=re.sub(r'([^\\])"', r'\g<1>\"', dString)
+                    dString = re.sub(r'([^\\])"', r'\g<1>\"', dString)
                     s = '%%feature("docstring") OpenMM::%s::%s "%s";' \
                        % (shortClassName, methName, dString)
                     self.fOutDocstring.write("%s\n" % s)
-                self.fOutDocstring.write("\n\n")
-        #print "Done write Docstring info\n"
 
+                self.fOutDocstring.write("\n\n")
 
 
     def writeSwigFile(self):

wrappers/python/src/swig_doxygen/swig_lib/python/extend.i

@@ -51,33 +51,33 @@
 
 
   %pythoncode %{
-    def getState(self,
-                 getPositions=False,
-                 getVelocities=False,
-                 getForces=False,
-                 getEnergy=False,
-                 getParameters=False,
-                 enforcePeriodicBox=False,
-                 groups=-1):
-        """
-        getState(self, getPositions=False, getVelocities=False, getForces=False,
-                 getEnergy=False, getParameters=False, enforcePeriodicBox=False,
-                 groups=-1) -> State
-
-        Get a State object recording the current state information stored in this context.
-
-        Parameters:
-         - getPositions (bool=False) whether to store particle positions in the State
-         - getVelocities (bool=False) whether to store particle velocities in the State
-         - getForces (bool=False) whether to store the forces acting on particles in the State
-         - getEnergy (bool=False) whether to store potential and kinetic energy in the State
-         - getParameter (bool=False) whether to store context parameters in the State
-         - enforcePeriodicBox (bool=False) if false, the position of each particle will be whatever position is stored in the Context, regardless of periodic boundary conditions.  If true, particle positions will be translated so the center of every molecule lies in the same periodic box.
-         - groups (set={0,1,2,...,31}) a set of indices for which force groups
-           to include when computing forces and energies. The default value
-           includes all groups. groups can also be passed as an unsigned integer
-           interpreted as a bitmask, in which case group i will be included if
-           (groups&(1<<i)) != 0.
+    def getState(self, getPositions=False, getVelocities=False,
+                 getForces=False, getEnergy=False, getParameters=False,
+                 enforcePeriodicBox=False, groups=-1):
+        """Get a State object recording the current state information stored in this context.
+
+        Parameters
+        ----------
+        getPositions : bool=False
+            whether to store particle positions in the State
+        getVelocities : bool=False
+            whether to store particle velocities in the State
+        getForces : bool=False
+            whether to store the forces acting on particles in the State
+        getEnergy : bool=False
+            whether to store potential and kinetic energy in the State
+        getParameter : bool=False
+            whether to store context parameters in the State
+        enforcePeriodicBox : bool=False
+            if false, the position of each particle will be whatever position
+            is stored in the Context, regardless of periodic boundary conditions.
+            If true, particle positions will be translated so the center of
+            every molecule lies in the same periodic box.
+        groups : set={0,1,2,...,31}
+            a set of indices for which force groups to include when computing
+            forces and energies. The default value includes all groups. groups
+            can also be passed as an unsigned integer interpreted as a bitmask,
+            in which case group i will be included if (groups&(1<<i)) != 0.
         """
         getP, getV, getF, getE, getPa, enforcePeriodic = map(bool,
             (getPositions, getVelocities, getForces, getEnergy, getParameters,
@@ -208,33 +208,32 @@ Parameters:
                  getParameters=False,
                  enforcePeriodicBox=False,
                  groups=-1):
-        """
-        getState(self,
-                 copy,
-                 getPositions = False,
-                 getVelocities = False,
-                 getForces = False,
-                 getEnergy = False,
-                 getParameters = False,
-                 enforcePeriodicBox = False,
-                 groups = -1)
-              -> State
-
-        Get a State object recording the current state information about one copy of the system.
-
-        Parameters:
-         - copy (int) the index of the copy for which to retrieve state information
-         - getPositions (bool=False) whether to store particle positions in the State
-         - getVelocities (bool=False) whether to store particle velocities in the State
-         - getForces (bool=False) whether to store the forces acting on particles in the State
-         - getEnergy (bool=False) whether to store potential and kinetic energy in the State
-         - getParameter (bool=False) whether to store context parameters in the State
-         - enforcePeriodicBox (bool=False) if false, the position of each particle will be whatever position is stored in the Context, regardless of periodic boundary conditions.  If true, particle positions will be translated so the center of every molecule lies in the same periodic box.
-         - groups (set={0,1,2,...,31}) a set of indices for which force groups
-           to include when computing forces and energies. The default value
-           includes all groups. groups can also be passed as an unsigned integer
-           interpreted as a bitmask, in which case group i will be included if
-           (groups&(1<<i)) != 0.
+        """Get a State object recording the current state information about one copy of the system.
+
+        Parameters
+        ----------
+        copy : int
+            the index of the copy for which to retrieve state information
+        getPositions : bool=False
+            whether to store particle positions in the State
+        getVelocities : bool=False
+            whether to store particle velocities in the State
+        getForces : bool=False
+            whether to store the forces acting on particles in the State
+        getEnergy : bool=False
+            whether to store potential and kinetic energy in the State
+        getParameter : bool=False
+            whether to store context parameters in the State
+        enforcePeriodicBox : bool=False
+            if false, the position of each particle will be whatever position
+            is stored in the Context, regardless of periodic boundary conditions.
+            If true, particle positions will be translated so the center of
+            every molecule lies in the same periodic box.
+        groups : set={0,1,2,...,31}
+            a set of indices for which force groups to include when computing
+            forces and energies. The default value includes all groups. groups
+            can also be passed as an unsigned integer interpreted as a bitmask,
+            in which case group i will be included if (groups&(1<<i)) != 0.
         """
         getP, getV, getF, getE, getPa, enforcePeriodic = map(bool,
             (getPositions, getVelocities, getForces, getEnergy, getParameters,