pyGSTi
pyGSTi copied to clipboard
sandialabs
Constructor documentation duplication
I am going to preface this by acknowledging that there are many different perspectives on this, but wanted to open up a discussion to establish what our 'house style' ought to be.
See below an example of one of the classes from workspaceplots.py:
class MatrixPlot(WorkspacePlot):
"""
Plot of a general matrix using colored boxes
Parameters
----------
ws : Workspace
The containing (parent) workspace.
matrix : ndarray
The operation matrix data to display.
color_min : float
Color scale minimum.
color_max : float
Color scale maximum.
xlabels : list, optional
List of (str) box labels along the x-axis.
ylabels : list, optional
List of (str) box labels along the y-axis.
xlabel : str, optional
A x-axis label for the plot.
ylabel : str, optional
A y-axis label for the plot.
box_labels : bool, optional
Whether box labels are displayed.
colorbar : bool optional
Whether to display a color bar to the right of the box plot. If None,
then a colorbar is displayed when `box_labels == False`.
colormap : Colormap, optional
A color map object used to convert the numerical matrix values into
colors.
prec : int or {'compact','compacthp'}, optional
Precision for box labels. Only relevant when box_labels == True. Allowed
values are:
- 'compact' = round to nearest whole number using at most 3 characters
- 'compacthp' = show as much precision as possible using at most 3 characters
- int >= 0 = fixed precision given by int
- int < 0 = number of significant figures given by -int
scale : float, optional
Scaling factor to adjust the size of the final figure.
grid : {"white","black",None}
What color grid lines, if any, to add to the plot. Advanced usage
allows the addition of `:N` where `N` is an integer giving the line
width.
"""
def __init__(self, ws, matrix, color_min=-1.0, color_max=1.0,
xlabels=None, ylabels=None, xlabel=None, ylabel=None,
box_labels=False, colorbar=None, colormap=None, prec=0,
scale=1.0, grid="black"):
"""
Creates a color box plot of a matrix using the given color map.
This can be a useful way to display large matrices which have so many
entries that their entries cannot easily fit within the width of a page.
Parameters
----------
matrix : ndarray
The operation matrix data to display.
color_min, color_max : float, optional
Min and max values of the color scale.
xlabels, ylabels: list, optional
List of (str) box labels for each axis.
xlabel : str, optional
A x-axis label for the plot.
ylabel : str, optional
A y-axis label for the plot.
box_labels : bool, optional
Whether box labels are displayed.
colorbar : bool optional
Whether to display a color bar to the right of the box plot. If None,
then a colorbar is displayed when `box_labels == False`.
colormap : Colormap, optional
A color map object used to convert the numerical matrix values into
colors.
prec : int or {'compact','compacthp'}, optional
Precision for box labels. Only relevant when box_labels == True. Allowed
values are:
- 'compact' = round to nearest whole number using at most 3 characters
- 'compacthp' = show as much precision as possible using at most 3 characters
- int >= 0 = fixed precision given by int
- int < 0 = number of significant figures given by -int
scale : float, optional
Scaling factor to adjust the size of the final figure.
grid : {"white","black",None}
What color grid lines, if any, to add to the plot. Advanced usage
allows the addition of `:N` where `N` is an integer giving the line
width.
"""
super(MatrixPlot, self).__init__(ws, self._create, matrix, color_min, color_max,
xlabels, ylabels, xlabel, ylabel,
box_labels, colorbar, colormap, prec, scale, grid)
def _create(self, matrix, color_min, color_max,
xlabels, ylabels, xlabel, ylabel,
box_labels, colorbar, colormap, prec, scale, grid):
if colormap is None:
colormap = _colormaps.DivergingColormap(vmin=color_min, vmax=color_max)
ret = _matrix_color_boxplot(
matrix, xlabels, ylabels, xlabel, ylabel,
box_labels, None, colorbar, colormap, prec, scale, grid=grid)
return ret
The thing I would like to draw attention to is that the docstring for the class nearly exactly mirrors that if the __init__. Not all style guides agree here regarding whether the constructor documentation should be associated with the class or with the __init__ (i.e. there is a principled reason why some folks opt to have both serving slightly different purposes), but I'm personally partial to the perspective that you should pick one or the other, but not both (and I am agnostic as to which).
TLDR: There is a lot of duplicated documentation here and I think we should prune it, both to cut down the length of the file, but also to reduce the number of places where we can make a mistake and fail to edit both instances of the docstrings when making a change. Thoughts?
Yea this (and related topics) is something we've discussed several times. The Google style guide suggests that the parameters should only be described in __init__, whereas the class docstring should just give an overview of the class (and also any public attributes, which we rarely document).
I am generally for removing duplicated documentation - it greatly increases our ability to maintain the code. Basically any time we find ourselves copy-pasting docstrings, I would rather do something else. One thing I've been playing around with in other projects is using See Also blocks or just saying that other parameters are documented in :meth:some_other_func. This has the disadvantage of having to manually searching for that function while developing (although IDEs may lower this barrier some), but has the advantage that Sphinx will create hyperlinks to that function in the autogenerated documentation. This is particularly helpful for parameters that are just being plumbed through to other functions. As my current goal basically is to make the ReadTheDocs the one-stop shop for all pyGSTi documentation (both tutorials and API reference), the pros of doing this referencing/linking greatly outweigh the cons for me, but I'm open to being convinced otherwise.
I also remember @rileyjmurray talking about docstring templates at some point, but can't remember the exact context.
I'm also fine with removing duplicated documentation. In this case I think that straight-up deleting the class docstrings and keeping in __init__ is reasonable.
Responding to Stefan's last comment, here's a super simple decorator definition that can programmatically populate docstrings:
def set_docstring(docstr):
def assign(fn):
fn.__doc__ = docstr
return fn
return assign
Here's how I used it in other projects. The idea was to define an interface class that included a template docstring with a few blank spots, then subclasses would define a tuple of strings called that held content to substitute into the template.
class TaskInterface:
"""
Class for doing the useful thing.
"""
TEMPLATE_DOCSTR = """
Given x and y, approximately compute
the_thing(x, y).
%s
Parameters
------------
x : something
y : something
tol : float
%s
Returns
--------
val : something
%s
"""
# Dummy values
INTERFACE_FIELDS = (
"""
Use [the method].
""",
"""
We use an iterative method, and stop once [metric] falls below tol.
""",
"""
Note, "val" is guaranteed to satisfy [condition].
"""
)
DOCSTR = TEMPLATE_DOCSTR % INTERFACE_FIELDS
@set_docstring(DOCSTR)
def __call__(x, y, tol):
raise NotImplementedError()
