search

Geodels / bioLEC

Landscape Elevational Connectivity package https://biolec.readthedocs.io/
GNU General Public License v3.0
4 stars 3 forks source link

JOSS Review: Improvements to docstrings related to connectivity/boundary conditions #6

Closed kbarnhart closed 5 years ago

kbarnhart commented 5 years ago

Comments from my JOSS Review regarding the contents of the documentation.

@tristan-salles Overall I found the docstrings (and their representation on RTFD) really nice. There was, however, one case where I couldn't figure something out from the docs that I think I should have been able to figure out.

I was not able to figure out the periodic and symmetric keyword arguments without looking at the figure presented in the notebook bioLEC1-genericCase.ipynb. I would have liked to understand the values from the docstring alone, or to have an image reference to the notebook.

These two keywords brought up a few thoughts: 1) I think they are incompatible with one another, right? If so, the code presently doesn't raise an error if both are set to true. 2) (Perhaps not for this present release, but) I think it might make sense to consider each boundary (North, South, East, and West) independently. That is, you could have East and West boundaries as periodic, North as symmetric, and South as "normal".

In addition, I think it would be worth using a different name for the keyword argument connected. As I understand it, this pertains to D4/D8 connectivity. Perhaps "connectivity" (with valid values "axial" and "both") or "diagonals" with valid values (False, True). At the least, I'd recommend using the words D4 and D8 in the docstring and making the y direction paneling label in the bioLEC1-genericCase.ipynb figure reflect the keyword argument.

tristan-salles commented 5 years ago

As pointed by @kbarnhart, the periodic and symmetric boundary conditions are incompatible as they are applied to all boundaries in this current version of bioLEC. I have added a couple of lines in the code to raise a warning when both conditions are set to True. I have also added a link in the docstring to a figure in the documentation that illustrates both boundary conditions.

Also following @kbarnhart comments I have modified the keyword argument connected and changed it to diagonals with boolean values. I have also added the D4 and D8 words in the docstring plus I have modified the figure to reflect those changes.