-
-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[REVIEW]: Dynamax: A Python package for probabilistic state space modeling with JAX #7069
Comments
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:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
Software report:
Commit count by author:
|
Paper file info: 📄 Wordcount for ✅ The paper includes a |
License info: ✅ License found: |
|
Review checklist for @gdalleConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Review checklist for @thomaspinderConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
👋 @gdalle, @thomaspinder, could you please update us on how it's going with your reviews? |
@osorensen expecting to be done by September 1st. |
Haven't started it yet so I will probably need two more weeks. |
Dynamax ReviewFirstly, I'd like to congratulate the authors and contributors of Dynamax for creating a nicely designed package in JAX. Dynamax enables practitioners to easily fit state-space models (SSM), whilst simultaneously allowing researchers to implement their own custom SSM approaches. The repo's corresponding paper is well written and provides a clear and concise summary of the paper. Finally, I enjoyed reading the documentation of Dynamax; the large number of examples and use-cases is great for new users of the package. I have broken my review up into two sections. The first section lists the blocking issues I have that prevent me from being able to mark each item of the reviewer's checklist as complete. In the second section, I suggest some things that I would advise the authors to do in Dynamax; however, I do not consider these as blocking concerns. Significant issues
Failing testsFor me, running
as per the docs, threw errors on a Mac M1. I was using Python 3.10 and a fresh virtual environment. On this topic though, I see that you're Github testing workflow uses a pinned Python version and machine. It could be good to run your tests on all supported Python versions and a Mac and Linux machine. I have opened a PR with a suggestion that reflects this comment. Docstring coverageThe docstrings within Dynamax are inconsistent. Taking, for example, :param initial_mean: $m$
:param initial_covariance: $S$ It would be good to see more comprehensive documentation. To rigorously test documentation thresholds in the future, you may consider using 33.1% of classes/methods/functions are covered by tests. It would be good to see this increased. This was calculated with Broken documentationThe documentation is broken and many notebooks do not render - Example. My suggestion would be to use something like nbsphinx to execute the notebooks each time the documentation is built, and to fail when a notebook does not execute top-to-bottom. ### Incomplete contribution guidelines Your contribution guidelines are incomplete. To push a change, one would need to add and commit the change before pushing - this is missing from the document. ### Missing typing The package has no typing. Additionally, many functions use variable names which are very hard to interpret e.g., Incorrect typingSome of the typing used is incorrectly done. Take - initial_mean: Float[Array, "state_dim"]
+ initial_mean: Float[Array, " state_dim"] should be made. Suggested improvements
### Large repo size You have some very large git pack files:
You may consider running ### Tighter Python bounds I would suggest tighter Python bounds. Currently your lower bound is 3.6; a version which is considered end-of-life by the Python Org.: https://devguide.python.org/versions/#unsupported-versions Package managementIt is strongly advised to migrate |
Thanks a lot for your thorough review, @thomaspinder. @slinderman, you're welcome to start addressing the issues whenever you like. |
Thank you, @osorensen and @thomaspinder! We will start working on these issues and suggestions ASAP, and I'll keep you posted. |
Meanwhile I'm making my way through my own review. I will gather my remarks about code and documentation in two issues on the dynamax repo:
I might be limited by my very ancient Python skills for installation and testing of dynamax, so I'd appreciate a hand in figuring out the errors I observe. As for the paper itself, it is very clear and does a good job of introducing dynamax. I have three minor suggestions:
Can you give more details on how this is implemented API-wise? For instance, how generic can observation distributions be?
Would it be possible to provide concrete examples of what is missing from the previous libraries? Obviously JAX support is a big aspect, since I know that some of them are coded in Numpy (hmmlearn) or PyTorch (pomegranate)
What do you mean by sublinear time? Isn't it just (roughly speaking) total sequential time divided by amount of parallelism? |
I'm halfway through the documentation and a significant number of code examples are broken by the Numpy 2.0 release (about 1 per page). I think it would be a good idea to fix them before I make a second (complete) pass. |
@slinderman, ref the post by @gdalle, could you please follow up and ping us here when done? |
Hi @osorensen, thanks for the reminder. @gdalle, sorry for the rendering issue. I thought we had fixed this by pinning to Numpy<2.0, but somehow this slipped through the cracks. I will fix it and let you know when it's ready for another pass. |
@slinderman any updates on this? |
Hi @osorensen, sorry for the delay. We pinned Numpy < 2.0 and recreated the documentation, and now almost all of the rendering issues should be fixed. There is a known issue with the HMC notebook (see probml/dynamax#384), but the rest look good. Regarding the overall review progress, we have begun work on the feedback we received from @thomaspinder. It is taking more time than anticipated to fix the typing issues, but we aim to wrap that up in the coming weeks. We greatly appreciate the reviewers' feedback, and we appreciate their patience while we work to address their comments. |
Thanks for the update, @slinderman |
@slinderman thanks for the fixes. Can you tag a new release containing them, so that running the documentation examples does not require cloning the repo? |
Hi @gdalle, of course. Please see https://github.com/probml/dynamax/releases/tag/0.1.5, which is now available on PyPI as well. |
Submitting author: @slinderman (Scott Linderman)
Repository: https://github.com/probml/dynamax
Branch with paper.md (empty if default branch): paper
Version: v0.1.4
Editor: @osorensen
Reviewers: @thomaspinder, @gdalle
Archive: Pending
Status
Status badge code:
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
@thomaspinder & @gdalle, 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:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @osorensen 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 @gdalle
📝 Checklist for @thomaspinder
The text was updated successfully, but these errors were encountered: