openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
712 stars 38 forks source link

[REVIEW]: Taweret: a Python package for Bayesian model mixing #6175

Closed editorialbot closed 3 months ago

editorialbot commented 9 months ago

Submitting author: !--author-handle-->@ominusliticus<!--end-author-handle-- (Kevin Ingles) Repository: https://github.com/bandframework/Taweret.git Branch with paper.md (empty if default branch): joss-submission Version: 1.0.2 Editor: !--editor-->@rkurchin<!--end-editor-- Reviewers: @julienmalard, @gchure Archive: 10.5281/zenodo.11254359

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/43c5f0d033f24fb1c0a1ae00326a169b"><img src="https://joss.theoj.org/papers/43c5f0d033f24fb1c0a1ae00326a169b/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/43c5f0d033f24fb1c0a1ae00326a169b/status.svg)](https://joss.theoj.org/papers/43c5f0d033f24fb1c0a1ae00326a169b)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@julienmalard & @gchure, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @rkurchin know.

Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest

Checklists

📝 Checklist for @gchure

📝 Checklist for @julienmalard

editorialbot commented 9 months ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

For a list of things I can do to help you, just type:

@editorialbot commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@editorialbot generate pdf
editorialbot commented 9 months ago
Software report:

github.com/AlDanial/cloc v 1.88  T=0.71 s (76.5 files/s, 434739.9 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JSON                             2              0              0         293408
Python                          19            819           1731           2189
Jupyter Notebook                10              0           6359           1181
Markdown                         4            110              0            440
reStructuredText                12            142            136            206
TeX                              1             19              0            164
YAML                             3              4              3             64
DOS Batch                        1              8              1             26
make                             1              4              7              9
Bourne Shell                     1              0              0              5
-------------------------------------------------------------------------------
SUM:                            54           1106           8237         297692
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 9 months ago

Wordcount for paper.md is 2363

editorialbot commented 9 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1214/21-BA1287 is OK
- 10.1103/PhysRevC.106.044002 is OK
- 10.1088/1361-6471/abf1df is OK
- 10.1080/00401706.2023.2257765 is OK
- 10.1016/j.jmva.2019.104555 is OK
- 10.1214/17-BA1091 is OK
- 10.1175/MWR3441.1 is OK
- 10.3389/fnhum.2014.00457 is OK
- 10.1016/j.cpc.2021.108168 is OK

MISSING DOIs

- None

INVALID DOIs

- https://doi.org/10.1111/insr.12243 is INVALID because of 'https://doi.org/' prefix
editorialbot commented 9 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

gchure commented 9 months ago

Review checklist for @gchure

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

rkurchin commented 8 months ago

@ominusliticus, be sure to fix that one DOI when you have a moment.

ominusliticus commented 8 months ago

The DOI has been updated :+1:

gchure commented 8 months ago

Hi @rkurchin and @ominusliticus , I've finished what I can of the review for Taweret as of now. I've opened several issues with #51 and #52 currently blocking for acceptance (discussed below). My comments on the paper (#54) are minor and non-blocking--I will leave them up to the discretion of @rkurchin, but having a statement of contribution with this many co-first authors feels important.

Needed to finish review

To finish my review, I need to sufficiently assess functionality. To do so, I would like to be able to run and toy with the example models provided with the documentation, but I run into ptemcee errors when running a notebook locally. As I discuss in that issue, I suspect this is on my end but having ptemcee not be included within the requirements.txt file during installation I think makes it relevant to the review, i.e. if I ran into this error, end-users will certainly run into it as well.

Explanation of blocking changes

I view issues#51 and #52 as blocking because they are critical to use of the software. Installing Taweret is a bit of a bear as I have to clone the repository, add it to my PATH, and then hunt down a binary to run the regression trees. Unless I am missing something big (which is totally plausible), this could be greatly simplified for the end-user by packaging Taweret on pip and then defining a build_taweret command that does the full regression tree configuration. For comparison, cmdstanpy (a industry-standard library for Bayesian inference via Hamiltonian MCMC) has a function for installing the cmdstan toolchain in a platform-agnostic manner. Something similar for Taweret would enormously simplify installation for the end user.

Finally, I am concerned that the current version of Taweret does not have a CI/CD pipeline. The authors discuss this as a goal for the software in the paper, but I think the authors should really prioritize it. The tests are currently failing (see #52), which would have been caught with CI/CD in place.

rkurchin commented 8 months ago

Thanks, @gchure. @ominusliticus, take note!

@julienmalard, any sense when you'll be able to start working on your review?

julienmalard commented 8 months ago

Would 2-3 weeks from now be too late?

rkurchin commented 8 months ago

Earlier would certainly be better (even if you can only get started but maybe file a few issues that @ominusliticus would be able to start working on), but if you need to wait that long, that's okay.

ominusliticus commented 8 months ago

Hi all, my co-authors and I have started to address/implement @gchure comments/suggestions. Everything should be up-to-date this weekend.

rkurchin commented 8 months ago

@ominusliticus checking in on the status of these updates!

ominusliticus commented 8 months ago

@rkurchin we responded to @gchure 's comments today and included is suggestion. I will have the bot generate a new draft. The biggest thing is getting CI/CD working, which is the goal for this week.

ominusliticus commented 8 months ago

@editorialbot generate pdf

editorialbot commented 8 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

julienmalard commented 8 months ago

Review checklist for @julienmalard

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

gchure commented 8 months ago

Hi @rkurchin, I've played around with Taweret more and am happy with the software (good work to @ominusliticus and everyone else!). The remaining blocking issue on my end is i) implementation of CI/CD and ii) release of Taweret on PyPi. Once those are configured (and I can test installation one more time), I'll finish my review and recommend Taweret for publication in JOSS.

julienmalard commented 8 months ago

I have finished my initial review (see https://github.com/bandframework/Taweret/issues/56 and https://github.com/bandframework/Taweret/issues/55).

ominusliticus commented 7 months ago

Hi @julienmalard, @gchure and @rkurchin, thank you for your patience! I just wanted to post a quick update: we are nearing completion of making the python packages available via PyPI. Our playing around TestPyPI seems to be promising. We still have to update the documentation with all the changes we have made regarding installation instructions and CI/CD. We are hoping to able to finalize everything next week.

rkurchin commented 7 months ago

🔔 Hi @ominusliticus, just checking in on the progress here!

ominusliticus commented 7 months ago

Hey @rkurchin, we are having trouble building wheels for the one of the dependencies of Taweret. Trying to resolve as quickly as possible! The documentation has been updated with new installation instructions. Once the dependency is successfully built, we can publish the packages and have people pip install them.

julienmalard commented 7 months ago

@ominusliticus Let me know if you have any particular difficulties with building the wheel that I could help out with..

ominusliticus commented 6 months ago

@rkurchin @gchure @julienmalard we are happy to announce that all the recommended changes have been implemented, along with the corresponding changes to the documentation. Taweret is now pip installable!

We found throughout the CI/CD process that having a copy of OpenMPI was necessary for the test_trees.py to pass. So if you wish to run these tests yourself, please be sure to have an installation.

Thank you for your patience!

rkurchin commented 6 months ago

Great! @julienmalard and @gchure, please take a look and see if this suffices to finish up your review checklists!

gchure commented 6 months ago

Hi @rkurchin, I will make sure to look at this by the end of the week.

gchure commented 6 months ago

Looks like there's still an issue with pip installation on MacOS, I think due to mismatched versioning in the metadata for openBT. I've commented on this here.

gchure commented 6 months ago

Hi @rkurchin! The installation problem has been fixed, and I can mark my review as complete. Congrats to @ominusliticus and the rest of the authors on a nice piece of software.

rkurchin commented 6 months ago

Thanks @gchure! @julienmalard, what are your thoughts?

julienmalard commented 6 months ago

Installation looks good on my end! When running the tests, I get the following error: https://github.com/bandframework/Taweret/issues/80

rkurchin commented 5 months ago

Hi @julienmalard and @ominusliticus, checking in on the status of the issue reference above?

jcyannotty commented 5 months ago

Hi @rkurchin, we think all @julienmalard needs to do is update Taweret via pip install Taweret --upgrade. The openbtmixing package was not installed when the tests were run. The newest version of Taweret includes openbtmixing as a dependency, so things should be installed properly if Taweret is upgraded.

rkurchin commented 5 months ago

Hi @julienmalard, have you had a chance to try this?

rkurchin commented 5 months ago

Pinging @julienmalard on this again!

julienmalard commented 4 months ago

Yes, just tried again (see comment in issue).

jcyannotty commented 4 months ago

Thanks for trying the code again! The issue was related to the dependency, so we will close the issue as you have instructed.

julienmalard commented 4 months ago

Perfect; my review is now done.

rkurchin commented 4 months ago

Post-Review Checklist for Editor and Authors

Additional Author Tasks After Review is Complete

Editor Tasks Prior to Acceptance

rkurchin commented 4 months ago

@ominusliticus, we're almost ready to go here! I'll send some editorial comments when I can; in the meantime, you can get going on the other pieces of info I need, summarized in the list above.

rkurchin commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1214/21-BA1287 is OK
- 10.1103/PhysRevC.106.044002 is OK
- 10.1088/1361-6471/abf1df is OK
- 10.1080/00401706.2023.2257765 is OK
- 10.1016/j.jmva.2019.104555 is OK
- 10.1214/17-BA1091 is OK
- 10.1175/MWR3441.1 is OK
- 10.3389/fnhum.2014.00457 is OK
- 10.1016/j.cpc.2021.108168 is OK

MISSING DOIs

- No DOI given, and none found for title: \textttloo: Efficient leave-one-out cross-validati...
- No DOI given, and none found for title: SAMBA: SAndbox for Mixing using Bayesian Analysis
- No DOI given, and none found for title: BANDFramework: An Open-Source Framework for Bayesi...
- No DOI given, and none found for title: Multifaceted study of ultrarelativistic heavy ion ...
- Entry without DOI or title found
- No DOI given, and none found for title: Taweret: A Python Package for Bayesian Model Mixin...
- No DOI given, and none found for title: Open Bayesian Trees Project
- No DOI given, and none found for title: BMA: Bayesian Model Averaging

INVALID DOIs

- doi.org/10.1111/insr.12243 is INVALID because of 'doi.org/' prefix
rkurchin commented 4 months ago

@ominusliticus please fix up those DOI issues when you can!

rkurchin commented 4 months ago

@editorialbot generate pdf

editorialbot commented 4 months ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

ominusliticus commented 4 months ago

@rkurchin will do. We will have a meeting tomorrow to try and finalize the release version!

ominusliticus commented 4 months ago

Regarding the MISSING DOIs

I have pushed to commits

ominusliticus commented 4 months ago

@editorialbot check references

editorialbot commented 4 months ago
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1111/insr.12243 is OK
- 10.1214/21-BA1287 is OK
- 10.1103/PhysRevC.106.044002 is OK
- 10.1088/1361-6471/abf1df is OK
- 10.1080/00401706.2023.2257765 is OK
- 10.1016/j.jmva.2019.104555 is OK
- 10.1214/17-BA1091 is OK
- 10.1175/MWR3441.1 is OK
- 10.3389/fnhum.2014.00457 is OK
- 10.1016/j.cpc.2021.108168 is OK

MISSING DOIs

- No DOI given, and none found for title: \textttloo: Efficient leave-one-out cross-validati...
- No DOI given, and none found for title: SAMBA: SAndbox for Mixing using Bayesian Analysis
- No DOI given, and none found for title: BANDFramework: An Open-Source Framework for Bayesi...
- No DOI given, and none found for title: Multifaceted study of ultrarelativistic heavy ion ...
- No DOI given, and none found for title: Taweret: A Python Package for Bayesian Model Mixin...
- No DOI given, and none found for title: Open Bayesian Trees Project
- No DOI given, and none found for title: BMA: Bayesian Model Averaging

INVALID DOIs

- None
rkurchin commented 4 months ago

Some editorial comments on the manuscript:

References:

I will note that this paper is on the long side for JOSS. I don't think there's inherently a problem with that, but I certainly encourage you to make sure that the more pedagogical content is also incorporated into the package docs so it is easy for users to find and for package maintainers to keep updated! The paper will be somewhat of a static document, useful for citation of course, but not really able to be updated in any kind of incremental way.