JOSS Review Notes

[rtbs-dev]

Very cool library, and excellently organized.
Here's some notes from my review to assist you guys beyond the JOSS checklist:

Easier Quickstart examples

Location where change is recommended

Documentation homepage or replacing "Getting Started"

Recommended change

Move or copy the `tutorial.ipynb` into the `docs/` folder, preferably using it as the "getting started" page, and renaming that page to "theory" or "background".

No "quick to check" examples for your library are mentioned in the documentation, and the getting started page only actually shows code the user could write to match the framework's operation, but never imports prog_medels or demos anything. Scrolling past a load of math/diagrams to see your library in action isn't great, despite loving that the math/diagrams are there.

Development Environment Setup

Location where change is recommended

Developer's Guide

Recommended change

Split out the Dev Guide from the Project Plan, and provide the necessary steps to create a working dev environment.

While I don't expect pip install prog_models to set up dev dependencies, I have come to expect cloning and pip install -e . to do so, or at least provide an option of pip install prog_models[dev] if you prefer. Running your tutorial notebook, running unit-tests, any code-formatting you want devs to do, static type checking, etc. is only possible with ipykernel, pytest, black code formatter, mypy, etc. These need to be listed at the very least, preferably automated. Personally I recommend complying with recent PEPs and using pyproject.toml instead of setuptools or setup.py, which can be facilitated by e.g. the poetry library.

Testing Expectations

Location where change is recommended

Developer's Guide

Recommended change

Explicitly state the expected test suite output for the current version, or provide alternate testing commands to reduce developer visual noise.

Running the tests (after figuring out the above dev environment on my own) led to many warnings, and some failures. Happy to open another issue about the failures (all about missing data zip files from the dataset/ URLs), but even the issues were a huge list of things that I'm not sure whether they were important or not. Most appear to be warnings for future deprecations, which (if so) shouldn't be used in unit tests. My output looks like:

============================= 3 failed, 92 passed, 1 skipped, 73 warnings in 192.36s (0:03:12) ==============================

which, I generally expect to show "all tests passed" for the stable branch of your repository. 73 is a lot of warnings, so make sure you either document that as expected or change your test suite to reflect a "working" stack.

Other

Tensorflow

This isn't a documentation issue, but related to "environment setup". Presumably your physics-based models do not have a hard dependency on Tensorflow? This is a huge dependency, especially when I'm always installing into clean virtual environments (conda, pyenv, virtualenv, etc). 500+MB is a lot to download every time I want to use your otherwise pretty lightweight library! I highly recommend optional dependencies e.g. pip install prog_models[tf] for tensorflow or prog_models[all] for "all the goodies". This uses the same machinery as the dev environment dependencies I talked about above. Just make sure to check when importing/initializing the data-driven models (LSTMStateTransitionModel?) that the user has installed the right optional deps.

There's nothing else I can think of, but let me know if I can clarify any of the above, further!