-
Notifications
You must be signed in to change notification settings - Fork 83
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
JOSS review - Remarks on code #377
Comments
I tried again to run the code on the documentation home page and got the following error: import jax.numpy as jnp
import jax.random as jr
from dynamax.hidden_markov_model import GaussianHMM
key1, key2, key3 = jr.split(jr.PRNGKey(0), 3)
num_states = 3
emission_dim = 2
num_timesteps = 1000
# Make a Gaussian HMM and sample data from it
hmm = GaussianHMM(num_states, emission_dim)
true_params, _ = hmm.initialize(key1) Traceback (most recent call last):
File "/home/gdalle/Work/GitHub/Python/dynamax-test/test.py", line 12, in <module>
true_params, _ = hmm.initialize(key1)
^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/dynamax/hidden_markov_model/models/gaussian_hmm.py", line 649, in initialize
params["initial"], props["initial"] = self.initial_component.initialize(key1, method=method, initial_probs=initial_probs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/dynamax/hidden_markov_model/models/initial.py", line 45, in initialize
initial_probs = tfd.Dirichlet(self.initial_probs_concentration).sample(seed=this_key)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/distributions/distribution.py", line 1205, in sample
return self._call_sample_n(sample_shape, seed, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/distributions/distribution.py", line 1182, in _call_sample_n
samples = self._sample_n(
^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/distributions/dirichlet.py", line 233, in _sample_n
log_gamma_sample = gamma_lib.random_gamma(
^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/distributions/gamma.py", line 725, in random_gamma
return random_gamma_with_runtime(
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/distributions/gamma.py", line 718, in random_gamma_with_runtime
seed = samplers.sanitize_seed(seed, salt='random_gamma')
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/internal/samplers.py", line 144, in sanitize_seed
seed = fold_in(seed, salt)
^^^^^^^^^^^^^^^^^^^
File "/home/gdalle/Work/GitHub/Python/dynamax-test/.venv/lib/python3.12/site-packages/tensorflow_probability/substrates/jax/internal/samplers.py", line 186, in fold_in
seed, jnp.asarray(salt & np.uint32(2**32 - 1), dtype=SEED_DTYPE))
~~~~~^~~~~~~~~~~~~~~~~~~~~~
OverflowError: Python int too large to convert to C long I think it is because you haven't yet released the fixes and tagged v0.1.5. |
Hi @gdalle, I just pushed 0.1.5 to pypi and tagged it as the latest release. It has the numpy < 2.0 requirement and the test fixes. Sorry for missing this earlier! |
Ok I finally managed to run the example code without issue on 0.1.5! Here are a few remarks (keeping in mind that I don't usually code in Python):
|
Hi and congrats on the package!
I'm one of the reviewers for the JOSS paper you submitted, so here I'll list my questions and concerns about the code itself. This issue will be updated as my reading progresses so maybe don't start answering right away.
Warning
I am not a Python developer, my main language is Julia, so I won't be able to give much advice beyond what I observed when trying out the examples. In addition, I don't know the best practices in terms of package and version management, I just use mamba and then pip inside the mamba environment.
Design
Execution
Tests
I followed the testing workflow on the home page of the documentation and it yielded one failure.
First tutorial
When I try to run the tutorial notebook after naive installation, it fails to generate sample data due to a Numpy 2.0 breaking change. I'm not sure why pip allowed me to install Numpy 2.0 as a dependency, since it doesn't seem to be supported yet (#366).
When I manually ensure that numpy 1.26.4 is used instead, I get an error even earlier, right when I try to import dynamax
Related issues:
The text was updated successfully, but these errors were encountered: