search

manodeep / Corrfunc

⚡️⚡️⚡️Blazing fast correlation functions on the CPU.
https://corrfunc.readthedocs.io
MIT License
163 stars 50 forks source link

Replace np.float with np.float64 to fix numpy 1.20 deprecation #250

Closed lgarrison closed 3 years ago

lgarrison commented 3 years ago

np.float is an alias to Python's builtin float and is deprecated in Numpy 1.20. It looks like we've been using np.float when we want 64-bit behavior (and np.float32 when we want 32 bits), so I think for us the correct change is to replace np.float with np.float64, even though they may be semantically different in rare cases. Let's make sure the tests pass, for starters.

@manodeep Any reservations on this change?

More on the deprecation:

/mnt/home/lgarrison/corrfunc/Corrfunc/io.py:244: DeprecationWarning: `np.float` is a deprecated alias for the builtin `float`. To silence this warning, use `float` by itself. Doing this will not modify any behavior and is safe. If you specifically wanted the numpy scalar type, use `np.float64` here.
  Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
    def read_catalog(filebase=None, return_dtype=np.float):
pep8speaks commented 3 years ago

Hello @lgarrison! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2021-06-08 16:03:34 UTC
manodeep commented 3 years ago

Yes - definitely needs to happen. I will take a look tomorrow

manodeep commented 3 years ago

@lgarrison Thanks! I found one instance of np.float under the docs. All the other instances are under the paper directory - what do you reckon?

And will you please add an entry to the changelog?

manodeep commented 3 years ago

Holy crap - we still haven't release 2.4.0? Okay - that's on my task list to do this week then

lgarrison commented 3 years ago

Done! And yeah, let's release 2.4.0! Lots of good quality-of-life changes there. I'll try to merge #199 before we release; I think we still want that change.

manodeep commented 3 years ago

@lgarrison Should we add a GH-actions checking with numpy 1.20?

lgarrison commented 3 years ago

Probably, yeah. Actually, I would advocate for just testing two versions of Numpy: the latest, and the earliest we want to support (so 1.14?). A smaller test matrix reduces the time for all the tests to launch and run, and more importantly, reduces the chances of one of the dozens of tests hanging, requiring manual intervention.

Does that sound okay to you? If this needs more discussion, we can defer that to later and I'll just add 1.20 to the matrix for now.

manodeep commented 3 years ago

My approach has been to the "oldest", "mid-tier", and "latest" versions of numpy. So in this case, we can retire 1.16 and add 1.20 - I.e., test on numpy versions [1.14, 1.18, 1.20]

manodeep commented 2 years ago

@lgarrison And presumably this branch is safe to delete as well