desihub / desitarget

DESI Targeting
BSD 3-Clause "New" or "Revised" License
18 stars 23 forks source link

Remove deprecated np.int and np.float dependencies #797

Closed geordie666 closed 1 year ago

geordie666 commented 1 year ago

This PR addresses #790 (as well as fixing another couple of minor issues).

Sorry this took so long, I was expecting to need to update many occurrences of np.int and np.float and hadn't realized there were only a few.

TMI: The code I ran to check my casting choices were correct for the specific examples mentioned in #790 was:

Grr1 = 45.6
Grr2 = np.array([45.6])
Grr3 = np.array([45.6, 65.7])

for Grr in [Grr1, Grr2, Grr3]:
    print(Grr, isinstance(Grr, np.float))

<ipython-input-12-f43a48c0ecf8>:2: DeprecationWarning: `np.float` is a deprecated alias for the builtin `float`. To sile
nce 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#deprecation
s
  print(Grr, isinstance(Grr, np.float))

45.6 True
[45.6] False
[45.6 65.7] False

for Grr in [Grr1, Grr2, Grr3]:
    print(Grr, isinstance(Grr, float))

45.6 True
[45.6] False
[45.6 65.7] False

for Grr in [Grr1, Grr2, Grr3]:
    print(Grr, isinstance(Grr, np.float64))

45.6 False
[45.6] False
[45.6 65.7] False

and

nside_chunk1, nside1 = np.array([32, 32, 64]), np.array([16, 8, 16])
nside_chunk2, nside2 = np.array([32]), np.array([16])
nside_chunk3, nside3 = 32, 16
for nside_chunk, nside in zip([nside_chunk1, nside_chunk2, nside_chunk3], [nside1, nside2, nside3]):
    try:
        nchunk = 4**np.int(np.log2(nside_chunk) - np.log2(nside))
        print(nside_chunk, nchunk)
    except TypeError:
        print("Casting {} and {} wasn't possible anyway".format(nside_chunk, nside))

<ipython-input-4-9ca0a781b651>:6: DeprecationWarning: `np.int` is a deprecated alias for the builtin `int`. To silence this warning, use `int` by itself. Doing this will not modify any behavior and is safe. When replacing `np.int`, you may wish to use e.g. `np.int64` or `np.int32` to specify the precision. If you wish to review your current use, check the release note link for additional information.
Deprecated in NumPy 1.20; for more details and guidance: https://numpy.org/devdocs/release/1.20.0-notes.html#deprecations
  nchunk = 4**np.int(np.log2(nside_chunk) - np.log2(nside))
Casting [32 32 64] and [16  8 16] wasn't possible anyway
[32] 4
32 4

nside_chunk1, nside1 = np.array([32, 32, 64]), np.array([16, 8, 16])
nside_chunk2, nside2 = np.array([32]), np.array([16])
nside_chunk3, nside3 = 32, 16
for nside_chunk, nside in zip([nside_chunk1, nside_chunk2, nside_chunk3], [nside1, nside2, nside3]):
    try:
        nchunk = 4**int(np.log2(nside_chunk) - np.log2(nside))
        print(nside_chunk, nchunk)
    except TypeError:
        print("Casting {} and {} wasn't possible anyway".format(nside_chunk, nside))

Casting [32 32 64] and [16  8 16] wasn't possible anyway
[32] 4
32 4
coveralls commented 1 year ago

Coverage Status

Coverage decreased (-11.5%) to 44.728% when pulling 059fa96bfd1c7a6bb53e7e244ff9f06dc054ba1a on ADM-dep-fixes into 692bd313e66720232f46ea47b6ac7104631380ab on main.

geordie666 commented 1 year ago

Before we merge this I should try to work out why coverage has dropped so much. I'm not sure how I could have reduced coverage by 11% with only a dozen or so changes, but I'll investigate.

geordie666 commented 1 year ago

I'm confused about the drop in coverage. Running pytest --cov on Cori for this branch and for main create coverage reports that differ by only three lines, corresponding to the three lines of code I removed from randoms.py. So, at NERSC I obtain coverage results that are completely aligned with my expectation.

I'm therefore going to assume that the drop in coverage from 56.222% to 44.728% was a glitch in coveralls.

@sbailey: I think I'm ready to merge this branch, unless you have further feedback regarding my comments about why nside_chunk can't be array-like.

weaverba137 commented 1 year ago

@geordie666, I would not get into the habit of blaming coveralls on this. I have found them to be highly reliable. The more likely explanation is that tests are being skipped in the GitHub Actions environment.

geordie666 commented 1 year ago

@weaverba137: Sure, agreed. I used sloppy language. I should have stated "a glitch or change in something leading up to and including the coveralls step."