JOSS Review (@rgmyr): Python utilities documentation and style

[rgmyr]

The Python tools in scripts/utilities seem useful and functional, but some of the style and docstring conventions are inconsistent and unusual. Docstring coverage and consistency is the most relevant thing with respect to JOSS review guidelines, and I will give you credit for Functionality documentation once that is improved, but I'll also list some more minor points that I would like to see improved as a Python user:

• A brief overview of file contents/functionality somewhere (e.g., a README.md file within the directory). Given that it's just one flat directory, it'd be nice to have a single place to look and remember which stuff is in which file.
• More docstring coverage with a consistent, ideally PEP 8-complaint style (particularly for functions and classes). Popular choices are NumPy Style and Google Style. Also see PEP 257 -- Docstring Conventions. The main points are:
  • Docstrings (single- and multi-line) should use triple quotes """
  • They should be placed under (not above) the def or class definition line. (This way the docstring gets assigned to the __doc__ attribute of the object, which is what makes it viewable in notebooks and via text editor plugins.)
  • The closing """ of a multi-line docstring should be on a line by itself.
  • Arguments and return types should be documented on their own lines (not in a paragraph).
• Please consider following PEP 8 Naming Conventions more consistently: CamelCase for classes, under_scored for functions and class methods.
  • A related issue is name mangling, which occurs when leading double underscores are used in class method names (I'm looking at rescal_utilities.py in particular). I don't particularly care in this case, but it affects method access in a way which you may not be aware of. It is typically only used when you want to prevent subclasses from accessing or having name clashes with particular attributes or methods of the parent class. Single leading underscores are the preferred convention for non-public methods and instance variables.
• Please also consider following PEP 8 Import Conventions: standard library imports first, then third party imports, then local imports. This improves readability and makes it easier to see which packages are used and which files depend on each other.
  • Related: please never pollute the namespace by using from numpy import * (I'm looking at csp_creator.py in particular. import numpy as np and from numpy import int8, zeros are both acceptable alternatives). This is especially bad with large third party libraries, but it makes the code less easily decipherable even when importing from local files. Just avoid importing * from anything unless you have a good reason to (e.g., collecting the namespaces of multiple small, related files in the __init__.py of a package subdirectory)

When at least the first two points are satisfactorily addressed, I will be happy to mark Functionality documentation complete. The tutorials and other documentation files are great! My only small suggestion there is that docs/rescal-snow-inputs.md would be a easier to read if you used bold or verbatim text for the parameter and variable names.

[rgmyr]

@kellykochanski Thanks! Also I realize that some of the above points may seem nitpicky. I'm open to the argument that this is not primarily a python package, and I will be lenient as far as submission acceptance goes (although I do stand by them as suggestions, and hope that they're helpful in improving your Python work generally). The most important things to me are that docstrings use triple quotes, be placed under the def and class lines, and that there be some overview of utilities file contents.

[kellykochanski]

@rgmyr I appreciate the feedback. rescal_utilities has been developed by several people with different coding backgrounds and styles. During development, I focused on having a consistent level of functional documentation and examples, but did not set standards for any of the points you have mentioned above.

Most of these should be relatively straightforward changes.

[kellykochanski]

As a follow-up, however, Rescal-snow is not primarily a python package; the python components are tools for simulation setup, data management, and analysis, which we expect will be useful to some but not all users.

[kellykochanski]

@21dmohan @defaziogiancarlo You should take a look at these comments - a lot of rgmyr's feedback is relevant to your functions and will help you polish your future Python code.

(No need to change your code - I'm addressing these review comments)

[21dmohan]

Thank you for the feedback! I will definitely utilize this information and resources as I continue to code!

[kellykochanski]

@rgmyr I believe I've addressed your points in the JOSS-fixes branch:

• Added a README for scripts/utilities
• (Related: re-organized files to make their purposes easier to define and simplify dependencies)
• More descriptive doc attributes at the top of each file
• Added a handful of missing docstrings (left one-line "set" and "get" etc functions without)
• All docstrings now use triple quotes """ with consistent line breaks for multiline strings
• Inputs and outputs broken out onto separate lines in appropriate docstrings
• Classes now CamelCase, methods now under_scored
• double underscores generally replaced with single underscores to avoid unneeded name mangling
• instances of import * excised
• emphasized parameter and variable names in docs/rescal-snow-inputs.md

[rgmyr]

@kellykochanski Awesome, looks good to me. I will close this issue and mark Functionality documentation completed.