[JOSS Review] Functionality section

braniii commented 4 months ago

Hi @aadya940,

this is part of my review and will be updated. I will try to work through the review list in the next weeks.

[x] publish to PyPI or conda-forge: In the review criteria it is clearly mentioned, that Python packages should be pip installable. Having setup setup.py it should be easy to publish this package to pypi or conda-forge, or to make it at least installable from github via pip install git+https://github.com/aadya940/chainopy. This should also make it more likely that others will try out (and therefor cite) this package. For your optional deps, you could use extras_require in the setup.
[x] In the installation instruction (README) the Github link of this repository has a copy+paste error username instead of your username
[x] In requirements.txt cython is listed twice.
[x] Numpy is listed in both, requirements.txt and requirements_test.txt
[x] Fix installation via PyPI (see below), so include binaries for all supported python versions
[x] Include dependencies as requirements
[x] ~~Include optional dependencies [docs/tests], e.g. as extra_require, to allow for easy setup.~~ I saw you ticked it without including. But I guess this is no requirement and therefore, I remove this point.
[x] What are you using numba for? I cannot spot any usage in the code.
[x] Fix import issue (see below)

Kind regards Daniel Nagel

aadya940 commented 4 months ago

Will do it ASAP! Thanks :))

aadya940 commented 4 months ago

Hey, I've uploaded chainopy on pip:

https://pypi.org/project/chainopy/1.0/

NOTE : Due to the usage of Cython, the wheel is only compatible for linux platform at the moment, I'll compile and upload a windows wheel soon. Hope this works right noww

braniii commented 4 months ago

Hi, I've tried out to install chainopy via PyPI (under linux) and was not successful. Here a short example to reproduce the error.

# setting up a conda env
$ conda create -n chainopy python
$ conda activate chainopy

# install chainopy
$ python -m pip install chainopy
ERROR: Could not find a version that satisfies the requirement chainopy (from versions: none)
ERROR: No matching distribution found for chainopy

So I figured out, that the error is caused by the fact that the default python version is py3.12, but you have uploaded only binaries for py3.10.

aadya940 commented 3 months ago

Hi @braniii , the package is downloadable via pip install git+. I've also written a release.yml but it has some issues at the moment. :))

aadya940 commented 3 months ago

Hey @braniii , Is chainopy installable for you? pip install chainopy

braniii commented 3 months ago

Now I managed to pip install but I get following error:

# setting up a conda env
$ conda create -n chainopy python
$ conda activate chainopy

# install chainopy
$ python -m pip install chainopy

$ python
Python 3.12.4 | packaged by Anaconda, Inc. | (main, Jun 18 2024, 15:12:24) [GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import chainopy

A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.0.1 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled with NumPy 2.0.
Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.

If you are a user of the module, the easiest solution will be to
downgrade to 'numpy<2' or try to upgrade the affected module.
We expect that some modules will need time to support NumPy 2.

Traceback (most recent call last):  File "<stdin>", line 1, in <module>
  File "/home/.../chainopy/chainopy/__init__.py", line 1, in <module>
    from .markov_chain import MarkovChain
  File "/home/.../chainopy/chainopy/markov_chain.py", line 11, in <module>
    from ._backend import (
  File "/home/.../chainopy/chainopy/_backend/__init__.py", line 1, in <module>
    from . import _simulate
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/.../chainopy/chainopy/__init__.py", line 1, in <module>
    from .markov_chain import MarkovChain
  File "/home/.../chainopy/chainopy/markov_chain.py", line 11, in <module>
    from ._backend import (
  File "/home/.../chainopy/chainopy/_backend/__init__.py", line 1, in <module>
    from . import _simulate
  File "chainopy/_backend/_simulate.pyx", line 1, in init chainopy._backend._simulate
    import numpy as np
ImportError: numpy.core.multiarray failed to import (auto-generated because you didn't call 'numpy.import_array()' after cimporting numpy; use '<void>numpy._import_array' to disable if you are certain you don't need it).
>>>

PS: I get the same error using pip install git+...

braniii commented 3 months ago

Hey @braniii , Is chainopy installable for you? pip install chainopy

You fixed it for py3.12. But now there are no wheels for py3.11, py3.10 etc. So it works for py3.12 only.

aadya940 commented 3 months ago

@braniii I was using Numba earlier but I removed it as a part of a recent cleanup PR since the function it was used in wasn't contributing much to performance. Removed the word Numba from the paper also :))

aadya940 commented 3 months ago

@braniii Finally I've uploaded all 16 wheels :)), check it out here https://pypi.org/manage/project/chainopy/releases/

braniii commented 3 months ago

@braniii I was using Numba earlier but I removed it as a part of a recent cleanup PR since the function it was used in wasn't contributing much to performance. Removed the word Numba from the paper also :))

Thx, guess you should it remove as dependency as well.

braniii commented 3 months ago

Now I managed to pip install but I get following error:

# setting up a conda env
$ conda create -n chainopy python
$ conda activate chainopy

# install chainopy
$ python -m pip install chainopy

$ python
Python 3.12.4 | packaged by Anaconda, Inc. | (main, Jun 18 2024, 15:12:24) [GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import chainopy

A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.0.1 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled with NumPy 2.0.
Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.

If you are a user of the module, the easiest solution will be to
downgrade to 'numpy<2' or try to upgrade the affected module.
We expect that some modules will need time to support NumPy 2.

Traceback (most recent call last):  File "<stdin>", line 1, in <module>
  File "/home/.../chainopy/chainopy/__init__.py", line 1, in <module>
    from .markov_chain import MarkovChain
  File "/home/.../chainopy/chainopy/markov_chain.py", line 11, in <module>
    from ._backend import (
  File "/home/.../chainopy/chainopy/_backend/__init__.py", line 1, in <module>
    from . import _simulate
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/.../chainopy/chainopy/__init__.py", line 1, in <module>
    from .markov_chain import MarkovChain
  File "/home/.../chainopy/chainopy/markov_chain.py", line 11, in <module>
    from ._backend import (
  File "/home/.../chainopy/chainopy/_backend/__init__.py", line 1, in <module>
    from . import _simulate
  File "chainopy/_backend/_simulate.pyx", line 1, in init chainopy._backend._simulate
    import numpy as np
ImportError: numpy.core.multiarray failed to import (auto-generated because you didn't call 'numpy.import_array()' after cimporting numpy; use '<void>numpy._import_array' to disable if you are certain you don't need it).
>>>

PS: I get the same error using pip install git+...

For me this error persists and I cannot import/use chainopy.

aadya940 commented 3 months ago

@braniii

Hey, sorry we had an incompatibility with Numpy 2.0.0 which is fixed now. Can you retry?
Also, pydtmc might not work with numpy 2.0.0 (Incase you're going to run the notebook again locally).
We also removed support for python 3.9 since chainopy requires networkx and only networkx version 3.3 supports numpy 2.0.0 but networkx doesn't support python 3.9.
Please do the installation in a venv.
Please let me know if you have any issues, Thanks for your patience :))

braniii commented 3 months ago

@braniii

* Hey, sorry we had an incompatibility with Numpy 2.0.0 which is fixed now. Can you retry?

* Also, pydtmc might not work with numpy 2.0.0 (Incase you're going to run the notebook again locally).

* We also removed support for python 3.9 since chainopy requires networkx and only networkx version 3.3 supports numpy 2.0.0 but networkx doesn't support python 3.9.

* Please do the installation in a `venv`.

* Please let me know if you have any issues, Thanks for your patience :))

Sorry, it was an error on my side. Removing the pip cache, fixed the issue.

aadya940 commented 3 months ago

Sorry for this, What's the error message?

aadya940 / chainopy

[JOSS Review] Functionality section #17