Use different alg for two_byte_sum that fixes off-by-one error #315

taldcroft · 2025-01-06T17:25:05Z

Description

This fixes a bug in two_byte_sum where it is off by one for negative integers.

Interface impacts

None

Testing

Unit tests

Mac

(ska3-flight-2025.0rc2) ➜  mica git:(fix-two-byte-sum) git rev-parse --short HEAD
0ade721
(ska3-flight-2025.0rc2) ➜  mica git:(fix-two-byte-sum) pytest                    
================================================================= test session starts =================================================================
platform darwin -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0
rootdir: /Volumes/git
configfile: pytest.ini
plugins: doctestplus-1.3.0, anyio-4.7.0, timeout-2.3.1
collected 113 items                                                                                                                                   

mica/archive/tests/test_aca_dark_cal.py ..................                                                                                      [ 15%]
mica/archive/tests/test_aca_hdr3.py ..                                                                                                          [ 17%]
mica/archive/tests/test_aca_l0.py ...ss                                                                                                         [ 22%]
mica/archive/tests/test_asp_l1.py sssssss                                                                                                       [ 28%]
mica/archive/tests/test_cda.py ..............................................                                                                   [ 69%]
mica/archive/tests/test_obspar.py .                                                                                                             [ 69%]
mica/report/tests/test_report.py ss                                                                                                             [ 71%]
mica/report/tests/test_write_report.py s                                                                                                        [ 72%]
mica/starcheck/tests/test_catalog_fetches.py ...............                                                                                    [ 85%]
mica/stats/tests/test_acq_stats.py .ss                                                                                                          [ 88%]
mica/stats/tests/test_guide_stats.py .sss                                                                                                       [ 92%]
mica/vv/tests/test_vv.py sssssssss                                                                                                              [100%]

=========================================================== 87 passed, 26 skipped in 23.38s ===========================================================

Independent check of unit tests by Jean

Linux (with relatively uncontaminated ska3-masters)

(test) jeanconn-fido> pytest
==================================================================================== test session starts ====================================================================================
platform linux -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0
rootdir: /proj/sot/ska/jeanproj/git
configfile: pytest.ini
plugins: timeout-2.3.1, anyio-4.7.0
collected 113 items                                                                                                                                                                         

mica/archive/tests/test_aca_dark_cal.py ..................                                                                                                                            [ 15%]
mica/archive/tests/test_aca_hdr3.py ..                                                                                                                                                [ 17%]
mica/archive/tests/test_aca_l0.py .....                                                                                                                                               [ 22%]
mica/archive/tests/test_asp_l1.py .......                                                                                                                                             [ 28%]
mica/archive/tests/test_cda.py ..............................................                                                                                                         [ 69%]
mica/archive/tests/test_obspar.py .                                                                                                                                                   [ 69%]
mica/report/tests/test_report.py ..                                                                                                                                                   [ 71%]
mica/report/tests/test_write_report.py .                                                                                                                                              [ 72%]
mica/starcheck/tests/test_catalog_fetches.py ...............                                                                                                                          [ 85%]
mica/stats/tests/test_acq_stats.py ...                                                                                                                                                [ 88%]
mica/stats/tests/test_guide_stats.py ....                                                                                                                                             [ 92%]
mica/vv/tests/test_vv.py .........                                                                                                                                                    [100%]

===================================================================================== warnings summary ======================================================================================
mica/mica/archive/tests/test_asp_l1.py::test_update_l1_archive
  /fido.real/miniforge3/envs/test/lib/python3.12/pty.py:95: DeprecationWarning: This process (pid=219487) is multi-threaded, use of forkpty() may lead to deadlocks in the child.
    pid, fd = os.forkpty()

mica/mica/archive/tests/test_cda.py::test_get_proposal_abstract
mica/mica/archive/tests/test_cda.py::test_get_proposal_abstract
mica/mica/report/tests/test_write_report.py::test_write_reports
mica/mica/report/tests/test_write_report.py::test_write_reports
  /fido.real/miniforge3/envs/test/lib/python3.12/site-packages/bs4/builder/_lxml.py:124: DeprecationWarning: The 'strip_cdata' option of HTMLParser() has never done anything and will eventually be removed.
    parser = parser(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================================== 113 passed, 5 warnings in 546.98s (0:09:06)
(test) jeanconn-fido> git rev-parse HEAD
812d3f079115c5601bb9ebd39123985085cb9f52

Functional tests

This looks right.

from mica.archive import aca_hdr3
from ska_matplotlib import plot_cxctime
from cheta import fetch_sci

dat_mica = aca_hdr3.MSID("ccd_temp", "2012:004:12:00:00", "2012:005:12:00:00")
dat_fetch = fetch_sci.Msid("aacccdpt", "2012:004:12:00:00", "2012:005:12:00:00")

plot_cxctime(dat_fetch.times, dat_fetch.vals)
plot_cxctime(dat_mica.times, dat_mica.vals)

As a reviewer I, Jean, also just looked at this and this makes sense to me to appropriately cover the range of signed 16 bit ints with the new code.

In [17]: bytes0 = [128, 127]

In [18]: bytes1 = [0, 255]

In [19]: slot_data = Table([bytes0, bytes1], names=['bytes0', 'bytes1'])

In [20]: aca_hdr3.two_byte_sum(["bytes0", "bytes1"])(slot_data)
Out[20]: 
masked_array(data=[-32768, 32767],
             mask=[False, False],
       fill_value=999999,
            dtype=int16)

The previous code/output just didn't succeed in covering the range.

In [8]: aca_hdr3.two_byte_sum(["bytes0", "bytes1"])(slot_data)
Out[8]: 
<Column name='bytes0' dtype='int64' length=2>
-32767
 32767

jeanconn · 2025-01-07T14:10:43Z

This is just a draft and I haven't looked at the full git history, but it is complaining about ruff fixes that were fixed in #312 so probably needs a rebase.

mica/archive/tests/test_aca_hdr3.py

jeanconn · 2025-02-24T11:38:26Z

mica/archive/aca_hdr3.py

+        # Make a 2xN array, then transpose to Nx2, then flatten to 2N, then copy to
+        # get values continous in memory.
+        bytes8_2xN = np.ma.vstack([bytes0, bytes1], dtype=np.uint8)
+        bytes8 = bytes8_2xN.transpose().flatten().copy()
+
+        # Now view the 2N bytes as N 16-bit signed integers.
+        ints16 = np.ma.array(bytes8.data.view(">i2"), mask=bytes8.mask[::2])


Thanks for the fix! Sorry I was not paying enough attention to the way to transform bytes to ints in general. I do wonder, now that I'm paying more attention, if instead of ending up with transpose().flatten().copy() + view if an updated shift and cast would end up being more readable

ints16 = ((bytes0.astype(np.uint16) << 8) | bytes1).astype(np.int16)

What are your thoughts?

(though I suppose I'd need to figure out if there are actually masked values in there anywhere and what happens to them)

What you wrote might be more concise and (with more investigation and testing) might work. But this PR is already so far down in the weeds, let's just go with the already-tested version.

jeanconn

Looks good to me. My only concern is that it looks to me like the new code returns a masked array even if that wasn't supplied. I think for our current use cases we don't care.

taldcroft force-pushed the fix-two-byte-sum branch from 258ce59 to ebfc72a Compare January 28, 2025 16:59

Use different alg for two_byte_sum that fixes off-by-one error

038546c

taldcroft force-pushed the fix-two-byte-sum branch from ebfc72a to 038546c Compare January 28, 2025 17:00

taldcroft added 2 commits January 28, 2025 12:02

ruff format 0.9

351244a

Make fix that works for masked arrays

b122f6e

taldcroft requested a review from jeanconn January 29, 2025 11:28

Add a unit test

0ade721

taldcroft marked this pull request as ready for review January 29, 2025 11:54

jeanconn reviewed Feb 24, 2025

View reviewed changes

mica/archive/tests/test_aca_hdr3.py Outdated Show resolved Hide resolved

jeanconn reviewed Feb 24, 2025

View reviewed changes

Update test

812d3f0

jeanconn approved these changes Feb 24, 2025

View reviewed changes

taldcroft merged commit 67a794e into master Feb 24, 2025
2 checks passed

taldcroft deleted the fix-two-byte-sum branch February 24, 2025 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use different alg for two_byte_sum that fixes off-by-one error #315

Use different alg for two_byte_sum that fixes off-by-one error #315

taldcroft commented Jan 6, 2025 •

edited by jeanconn

Loading

jeanconn commented Jan 7, 2025 •

edited

Loading

jeanconn Feb 24, 2025

jeanconn Feb 24, 2025

taldcroft Feb 24, 2025

jeanconn left a comment

Use different alg for two_byte_sum that fixes off-by-one error #315

Use different alg for two_byte_sum that fixes off-by-one error #315

Conversation

taldcroft commented Jan 6, 2025 • edited by jeanconn Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

jeanconn commented Jan 7, 2025 • edited Loading

jeanconn Feb 24, 2025

Choose a reason for hiding this comment

jeanconn Feb 24, 2025

Choose a reason for hiding this comment

taldcroft Feb 24, 2025

Choose a reason for hiding this comment

jeanconn left a comment

Choose a reason for hiding this comment

taldcroft commented Jan 6, 2025 •

edited by jeanconn

Loading

jeanconn commented Jan 7, 2025 •

edited

Loading