[JOSS Review] Documentation

[braniii]

Hi @aadya940,

this is part of my review and will be updated the next days.

Here is a list with open tasks:

• [x] Host the documentation
• [x] Fix sphinx so that the API is included in the documentation
• [x] Add examples for the advertised functionality in the docs.
• [x] Would it be possible to include the API of the public methods, rather than of the private submodules?
• [x] Please add a short description of the 3 classes, including a short description of the attributes. Only a name without a type, e.g. epsilon is (at least for me) not easy to interpret.
• [x] Fix rendering issues of the README. in "the basics" section.

I've finally finished the review. If you have any questions, feel free to write me.

Kind regards Daniel Nagel

[braniii]

Here is a list with minor issues I found while reviewing this repository. I thought it would be helpful sharing them:

• [x] in test_markovchain.py line 73+74 and 88+89 are without functionality. Line 75 should be if not (fm_x is None and fm_y is None):, but having a deterministic input, I guess line 75+76 are never reached and should be deleted!? The same for line 88+89. If you want to run the same test with different input parameters you can use @pytest.mark.parametrize , see https://docs.pytest.org/en/6.2.x/parametrize.html

[aadya940]

Yes @braniii , you are correct regarding the test_markovchain.py, I'll fix it soon. As for the docs, there is an issue with readthedocs, they give a 404 error which I'm unable to solve however you'll be able to build the docs locally. Also, the docs are essentially generated using the docstring so you could just refer to those as of now.

Here are the steps to generate the docs locally:

Go to the project folder:

Step - 1:

pip install -r requirements_doc.txt

Step - 2:

sphinx-apidoc -o docs/source chainopy/

Step - 3:

cd docs
make html

Step - 4:

cd build/html
python -m http.server

The docs will be listed on the given port.

[aadya940]

As for uploading the package on pip, I'll do it ASAP, sorry for the delay :))

[braniii]

Yes @braniii , you are correct regarding the test_markovchain.py, I'll fix it soon. As for the docs, there is an issue with readthedocs, they give a 404 error which I'm unable to solve however you'll be able to build the docs locally. Also, the docs are essentially generated using the docstring so you could just refer to those as of now.

Here are the steps to generate the docs locally:

Go to the project folder:

Step - 1:

pip install -r requirements_doc.txt

Step - 2:

sphinx-apidoc -o docs/source chainopy/

Step - 3:

cd docs
make html

Step - 4:

cd build/html
python -m http.server

The docs will be listed on the given port.

I've managed to build the docs, but it seems that there are some markdown parsing issues. I will wait until the documentation is back online.

[aadya940]

Hi @braniii , So the docs are up and running now at : https://chainopy.readthedocs.io/en/latest/

However, I'll improve these more in the future, this is just a start so that our reviews don't get affected. :))

[braniii]

@aadya940 Sorry for this lengthy review process. But I've finally finished the review. If you have any further questions, feel free to write me.

[aadya940]

The review has been really helpful to target sections which were lacking before. Thanks for the comprehensive review :))

[braniii]

The review has been really helpful to target sections which were lacking before. Thanks for the comprehensive review :))

You are welcome :)

[aadya940]

@braniii For point three:

 Add examples for the advertised functionality in the docs.

I believe I've added plenty examples in https://github.com/aadya940/chainopy/blob/master/examples/demo.ipynb , So I've just referenced this file at the top of my sphinx documentation and hence am checking the box. Hope it is fine for you :))

[braniii]

@braniii For point three:

 Add examples for the advertised functionality in the docs.

I believe I've added plenty examples in https://github.com/aadya940/chainopy/blob/master/examples/demo.ipynb , So I've just referenced this file at the top of my sphinx documentation and hence am checking the box. Hope it is fine for you :))

The issue with githubs ipynb rendering engine is, that it is still not supported in the official app. Could you therefore simply include the notebook as a new page into your docs?

[aadya940]

Yes good idea, thanks

[braniii]

And regarding the

• Please add a short description of the 3 classes, including a short description of the attributes. Only a name without a type, e.g. epsilon is (at least for me) not easy to interpret.

It seems, that these attributes are now included once with description and once without. (see e.g. eigenvalues or epsilon)

[braniii]

@aadya940 please tick off only issues you've fixed. Both, the examples are still missing and parts of the README are not rendered.

[aadya940]

@braniii Hello,

• The Jupyter Example Notebooks are now visible inside the docs under Examples section, you could check it out.
• I believe I've also fixed the rendering issue inside the README part.
• Attributes like epsilon being documented twice seems like a Sphinx issue since it doesn't behave well with __slots__, check it out here : https://stackoverflow.com/questions/78789259/how-to-prevent-sphinx-from-displaying-duplicate-attributes-in-class-documentatio Could we skip it for now till I come up with a good solution?

I hope everything seems good now :)) , Sorry for the delay.

[braniii]

Looks good.