JOSS Review -- Failing test_trees

[gchure]

Hi! I'm reviewing your paper for JOSS and am working my way through assessing testing.

I cannot get the tests to pass for test_trees.py. It is a mix of error modes, but here is my summary:

1. test_mixing and test_predict fail when making draws in _read_in_preds with ValueError: need at least one array to concatenate.
2. test_predict_wts fails with same ValueError, in the _read_in_wts function.
3. test_sigma fails with AttributeError in returning the _posterior hidden attribute.AttributeError: 'Trees' object has no attribute '_posterior'. Did you mean 'posterior'?

I've cloned Taweret as described on the repository installation instructions, including installing open-mpi and OpenBT as described in the dependencies. I am running Python 3.10.9 on macOS-14.1.1-arm64-arm-64bit. Here is a complete report of pytest on my machine: taweret_report.txt

Finally, it does not look like you are using any CI/CD on the Taweret main branch, which would be ideal. Is there a reason you are not using CI/CD such as Travis or through GitHub Actions? If there is a specific reason (which I bet there is), I'm totally fine with that. I do think, however, that you must disclose in the documentation how often you run tests during development. This is not only important for reliability and reproducibility, but will help other developers who want to contribute to Taweret.

[ominusliticus]

Hey! We have not been able to reproduce the error, but are fairly certain the errors arise from how OpenBT is installed. To circumvent the difficulty of making sure python finds the correct paths, we plan to package OpenBT as a wheel on the python package index (PyPI), and make it a dependency of Taweret so everything is pip-installable. The OpenBT repo will itself get a CI/CD that deploys the wheels to the index, as well as Taweret.

The reason for not including CI/CD is that we were short on time. We instead checked that each test worked on our respective devices, and when they did were fairly hopeful it would work. So we are working to add the CI/CD with GitHub Actions

[ominusliticus]

Hey @gchure, we are working on revamping the OpenBT dependency. Right now, it's set up so that a prebuilt binary is download with the openbtmixing package. The OpenBT package binary depends on an OpenMPI implementation (whose libraries ship with the binary), that may not be what users wish to use. So this is a customization point we wish to have, which will disable the plug-and-play spirit or pip-installable packages. Instead, it will be an opt in feature for only those who wish to use OpenBT

Our other reviewer seems to think that the openbtmixing dependency having issues is not a blocking change: link.

We have CI now for Taweret. We are removing some OpenBT specific test to the OpenBT repo, and things are still in flux.
Although CD is implemented for the Taweret package (tagged releases are pushed to PyPI), this will probably change in accordance with our parent's (BAND Org) future decisions.

All that to say, we hope your concerns are/have been addresses, and that this issue can be close

[gchure]

Thanks for the update. I can say this issue is resolved and will close it now.