geodels / biolec Goto Github PK

Another comment related to my JOSS Review...

There are a number of places in the docs where I expected a link to the api documentation. For example the landscapeConnectivity() at the top of the Quickstart.

The package is overall really well linked, this sort of issue was the only one where I wanted links that weren't there. I find that this sort of link helps a lot when one is trying to teach themself from the documentation.

The version described in the JOSS submission is 1.0.0

There are presently no tags or releases listed on GitHub.

My assumption is that @tristan-salles is intentionally waiting to make changes indicated in review before tagging.

As part of my JOSS Review I've looked through the readme.md, the read the docs page, the source code, and the notebooks. I've found that the installation instructions are duplicated across these locations.

I can understand wanting to make the installation instructions easily accessible. My experience, however, is that it is much better to have the instructions in one place and provide links. That way, when installation instructions change, you don't need to change all the files.

@tristan-salles feel free to make modifications based on this recommendation or completely ignore it.

@tristan-salles I thought that the statement of need was very clear in the the paper.md but not as clear on the RTFD page.

link to JOSS Review

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.

As part of my JOSS Review I'm testing the installation instructions. As I'll note in my review in that repo I'm testing the developer install (but not the other install instructions).

I found a few small errors in the install and testing. A forthcoming PR will describe the minor fixes that worked for me.

Another issue as part of my JOSS Review

I cloned the repo and tried to run the notebooks locally.

After running jupyter notebook from the top level Notebooks directory I opened the 0-StartHere.ipynb notebooks. I then ran into 404 errors with the links to the other notebooks. I assume this is because the are actually located in bioLEC/src/bioLEC/Notebooks

I'd recommend that there is one place for the file structure where the notebooks are located. If for some reason related to Docker that means they need to be in both of these places, perhaps make a note on the ReadTheDocs regarding how to run the notebooks locally).

I poked around in the repo trying to figure out a fix and found the documentation.py file. This may or not matter, but it was not at all clear to me whether the file is vestigial or whether I should have been able to use it to solve my 404 error issues.

As part of my JOSS Review I'm testing the notebooks.

I couldn't get the parts that related to lavavu to run locally---but the rest of the notebooks worked for me just fine.

On Binder I got unexpected results for the cells that relate to lavavu (for example, the following screenshot).

I tried this on both Safari and Chrome with the same failed results.