sot / chandra_aca

Chandra Aspect Camera Tools
https://sot.github.io/chandra_aca
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

[Speedy]: fix ellipsis in ACAImage and squash warning about integer arrays #167

Closed javierggt closed 9 months ago

javierggt commented 10 months ago

Description

This PRs fixes issues found in python 3.11 (Speedy):

NOTE: while working on this, I found some unrelated issues I have not fixed. in this PR

Fixes #166 Fixes #165

Interface impacts

Testing

Unit tests

Independent check of unit tests by @taldcroft

Functional tests

No functional testing.

taldcroft commented 9 months ago

@javierggt - my main question is if you understand why there are overflows (out of bounds) happening here that trigger the deprecation. Without understanding the code in detail, decom should by definition have predictable output values that are contained within well-specified bounds.

taldcroft commented 9 months ago

@javierggt - this needs a rebase now after #169. And hopefully the "fails independently of ..." comments will go away?

taldcroft commented 9 months ago

Unrelated to this PR, but for my future benefit can you add a comment line shown below.

From:

        out_rc = [None, None]  # New [row0, col0]

To:

        # These are new [row0, col0] values for the __getitem__ output. If either is left at None 
        # then the downstream code uses the original row0 or col0 value, respectively.
        out_rc = [None, None]
javierggt commented 9 months ago

my main question is if you understand why there are overflows (out of bounds) happening here that trigger the deprecation. Without understanding the code in detail, decom should by definition have predictable output values that are contained within well-specified bounds.

Yes.

The issue is not with the field size, but with converting from signed int8 to unsigned int8. I just took the easy way of replacing all instances where I was setting the dtype, although it was not necessary. Basically, I was converting numbers from np.int8 to np.uint8 by doing something equivalent to:

uval = np.array(val, dtype=np.uint8)

and changed it to this (based on their suggestion)

uval = np.array(val).astype(np.uint8)

One can reproduce this warning doing this:

from chandra_aca import maude_decom
import numpy as np
bits = b'\x8b'
bits = maude_decom._unpack("b", bits)
np.uint8(bits[0]).astype(np.int8)

Just now I looked at it again, and decided to fix in a different way so the relevant numbers do not require conversion, and I will fix it only where it is actually needed:

from chandra_aca import maude_decom
import numpy as np
bits = b'\x8b'
bits = maude_decom._unpack("B", bits)
np.uint8(bits[0]).astype(np.int8)
javierggt commented 9 months ago

@taldcroft I rebsaed on master, made some changes (mentioned in comments), and force-pushed.