PsrSigSim / PsrSigSim

The first version of our pulsar signal simulator.
MIT License
20 stars 21 forks source link

Module missing from install_requires and conflicts with `pint` library. #156

Closed JBorrow closed 3 years ago

JBorrow commented 4 years ago

In a clean install on python3.8.0, from pip, I get the following error:

Traceback (most recent call last):
  File "getting_started.py", line 4, in <module>
    import psrsigsim as pss 
  File "/private/tmp/env/lib/python3.8/site-packages/psrsigsim/__init__.py", line 15, in <module>
    from . import io
  File "/private/tmp/env/lib/python3.8/site-packages/psrsigsim/io/__init__.py", line 4, in <module>
    from .psrfits import PSRFITS
  File "/private/tmp/env/lib/python3.8/site-packages/psrsigsim/io/psrfits.py", line 7, in <module>
    from packaging import version
ModuleNotFoundError: No module named 'packaging'

This is of course resolved by installing packaging, but this should really be part of the install_requires.

One of your dependencies, pint-pulsar, has a concerning name conflict with the very popular pint symbolic unit library. This caused very confusing problems for me when running the tests. If this package is not really required (although based on its name, I guess it is), then it would be good to remove it. If not, you should definitely point this problem out clearly in https://psrsigsim.readthedocs.io/en/latest/contributing.html.

This issue is part of my review for JOSS.

paulthebaker commented 4 years ago

In anaconda python 3.8, all available astropy versions are 4.0+. Astropy v3 should work with Python 3.8, but I can't check this in my usual workflow. I can't find an official list of supported python versions for Astropy v3.2 after quick search. Are we sure it includes v3.8?

We appear to be held at astropy<4 by nanograv/PINT#651. Does this hold us at python<3.8 too?

In setup.py we specify python 3.5, 3.6, and 3.7 as compatible versions and not python 3.8. Do we need to add v3.8?

JBorrow commented 4 years ago

Python3.8 compatibility is not critical right now, but I would recommend you add it when you can. My major concern is the name of pint - installing this alongside the pint unit library causes everything to break. As this is a requirement of your code and people will probably have pint installed already, it would be good to either make a strong note of this in the docs, or to remove it as a dependency if you can get away with it.

bshapiroalbert commented 4 years ago

@paulthebaker you are correct that we were held to Astropy v3 by PINT, but I've just checked and it appears that they have updated their requirements for Astropy to: astropy>=3.2,!=4.0.1,!=4.0.1.post1, so I believe we could use the same requirements which would allow us to use python 3.8, but I should check (and @Hazboun6 may know) if this will cause issues with other packages currently required, particularly pdat and fitsio, or if it will cause issues with anything that currently relies on Astropy.

We do require pint-pulsar to handle a number of more complex calculations as other saving utilities related to the io class. I don't think we can get away with removing it as a dependency unfortunately, so we will make sure to make a strong note of this issue with the pint unit library in the docs.

Hazboun6 commented 4 years ago

@bshapiroalbert I believe that the != means "not equal to", so unfortunately I don't think they have fixed this. In fact I recently ran into the same issue that @JBorrow saw in Issue #157 (for a different project) where astropy 4.0 was installed automatically and then threw a number of errors.

[Edit] Sorry I misunderstood @bshapiroalbert . I didn't realize there were versions >4.0.1.post1.

Hazboun6 commented 4 years ago

I think we are going to need to just make a strong note of the Pint namespace conflict. As @bshapiroalbert noted, there is necessary functionality in the package and we are only going to require more of it as we add features.

There are ways around this, for instance we could change where Pint is installed, but my guess is actually that pint-pulsar is more likely to be installed by a user. One interesting point is that none of the developers use the more popular pint because the Quantity functionality is largely duplicated in astropy.

At least I can't think of a clean workaround.

Hazboun6 commented 3 years ago

I've resolved this Issue finally by adding a waring to the Installation page of the documentation.