Use CxoTime.linspace on get_aca_images for > 3hour range #181

jeanconn · 2024-10-17T19:25:26Z

Description

Use CxoTime.linspace on get_aca_images for > 3hour range

This is a possible and trivial implementation of @taldcroft 's thought at #174 (comment)

Interface impacts

This allows stop - start > 3 hours for the get_aca_images method.

Testing

This requires cxotime sot/cxotime#44

Unit tests

Mac

(ska3-flight-latest) flame:chandra_aca jean$ git rev-parse HEAD
3bcd3afc612953f4d6c4ac4e1206afecc032f8b5
(ska3-flight-latest) flame:chandra_aca jean$ pytest
============================================================ test session starts =============================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
PyQt5 5.15.9 -- Qt runtime 5.15.8 -- Qt compiled 5.15.8
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: astropy-0.11.0, qt-4.4.0, cov-5.0.0, timeout-2.2.0, remotedata-0.4.1, anyio-4.3.0, filter-subpackage-0.2.0, doctestplus-1.2.1, astropy-header-0.2.2, hypothesis-6.112.0, arraydiff-0.6.1, mock-3.14.0
collected 216 items                                                                                                                          

chandra_aca/tests/test_aca_image.py .................                                                                                  [  7%]
chandra_aca/tests/test_all.py ........................                                                                                 [ 18%]
chandra_aca/tests/test_attitude.py .............................................................                                       [ 47%]
chandra_aca/tests/test_dark_model.py ............                                                                                      [ 52%]
chandra_aca/tests/test_drift.py ..........................                                                                             [ 64%]
chandra_aca/tests/test_maude_decom.py ....................                                                                             [ 74%]
chandra_aca/tests/test_planets.py ...............                                                                                      [ 81%]
chandra_aca/tests/test_psf.py ...                                                                                                      [ 82%]
chandra_aca/tests/test_residuals.py ss...                                                                                              [ 84%]
chandra_aca/tests/test_star_probs.py .................................                                                                 [100%]

============================================================== warnings summary ==============================================================
chandra_aca/chandra_aca/tests/test_residuals.py::test_obc
  /Users/jean/miniforge3/envs/ska3-flight-latest/lib/python3.11/site-packages/django/utils/encoding.py:266: DeprecationWarning: 'locale.getdefaultlocale' is deprecated and slated for removal in Python 3.15. Use setlocale(), getencoding() and getlocale() instead.
    encoding = locale.getdefaultlocale()[1] or 'ascii'

chandra_aca/chandra_aca/tests/test_residuals.py::test_obc
  /Users/jean/miniforge3/envs/ska3-flight-latest/lib/python3.11/site-packages/django/http/request.py:1: DeprecationWarning: 'cgi' is deprecated and slated for removal in Python 3.13
    import cgi

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=========================================== 214 passed, 2 skipped, 2 warnings in 77.88s (0:01:17

Independent check of unit tests by [REVIEWER NAME]

[PLATFORM]:

Functional tests

Needs more functional testing.

chandra_aca/tests/test_maude_decom.py

chandra_aca/maude_decom.py

taldcroft · 2024-10-29T16:03:01Z

chandra_aca/maude_decom.py

-    return get_aca_packets(start, stop, level0=True, **maude_kwargs)
+    if CxoTime(stop) - CxoTime(start) > 1 * u.day:
+        raise ValueError("stop - start cannot be greater than 1 day")
+    maude_fetch_times = CxoTime.linspace(start, stop, step_max=3.0 * u.hour)


Make this default configurable by a module constant (equal to 3.0 * u.hour). Then the test can temporarily set that constant to something short in a little try / finally context manager.

Suggested change

maude_fetch_times = CxoTime.linspace(start, stop, step_max=3.0 * u.hour)

maude_fetch_times = CxoTime.linspace(start, stop, step_max=None)

if step_max is None:

step_max = MAUDE_FETCH_STEP_MAX

chandra_aca/maude_decom.py

taldcroft · 2024-10-29T16:08:29Z

chandra_aca/maude_decom.py

@@ -1209,24 +1211,36 @@ def _get_aca_packets(
    return table


-def get_aca_images(start, stop, **maude_kwargs):
+def get_aca_images(start: CxoTimeLike, stop: CxoTimeLike, **kwargs):
    """
    Fetch ACA image telemetry

    Parameters


Add documentation about the fetch intervals.

I didn't actually know that that meant so I just fluffed out the docstring a bit.

taldcroft · 2024-10-29T16:10:40Z

chandra_aca/maude_decom.py

+        )
+        for i in range(len(maude_fetch_times) - 1)
+    ]
+    return vstack(packet_stack)


One thing to confirm is that get_aca_packets is strict in the output to return items with start <= TIME < stop. I suspect that is the case but need to be sure.

chandra_aca/tests/test_maude_decom.py

taldcroft · 2024-10-29T16:21:39Z

chandra_aca/tests/test_maude_decom.py

+    assert abs(imgs[0]["TIME"] - CxoTime(start).secs) < 2
+    assert abs(imgs[-1]["TIME"] - CxoTime(stop).secs) < 2


Related to the previous comment, my understanding is that the TIME of returned images should be strictly between start and stop. Then TIME[0] - start should be between 0 and 4.1, and similar for TIME[1]. At least if my understanding is right, but if these tests are passing then I'm not sure.

chandra_aca/tests/test_maude_decom.py

jeanconn · 2024-11-07T04:03:03Z

chandra_aca/tests/test_maude_decom.py

+    imgs_start = maude_decom.get_aca_images(start, CxoTime(start) + 60 * u.s)
+    imgs_stop = maude_decom.get_aca_images(CxoTime(stop) - 60 * u.s, stop)
+    assert np.all(imgs[0] == imgs_start[0])
+    assert np.all(imgs[-1] == imgs_stop[-1])


I simplified the PR and removed the code I had to fetch extra time and re-filter the images by TIME. And now this isn't passing.

I fixed this.

Doesn't this suggest an upstream sort fix in get_aca_images?

The upstream fix would be to sort by time and slot. That's a question of API / expectations. In the current release the output is (empirically) sorted by [IMGNUM, TIME], so all the slot=0 are first and slot=7 are last. For chunked data this pattern is repeated for each chunk.

Overall I think we don't care as long as the images are sorted by TIME for a given IMGNUM. That is the only ordering that is currently guaranteed.

Right - though we added sorting on the mica side to be out.sort(keys=["TIME", "IMGNUM"]), so maybe maude_decom.get_aca_images should just do the same for consistency.

Add time linspace on get_aca_images for > 3hour range

3bcd3af

jeanconn commented Oct 17, 2024

View reviewed changes

chandra_aca/tests/test_maude_decom.py Outdated Show resolved Hide resolved

Put step_max right at the limit

5fa5161

taldcroft reviewed Oct 29, 2024

View reviewed changes

taldcroft marked this pull request as ready for review October 29, 2024 16:29

jeanconn added 5 commits November 5, 2024 15:10

Improve testing for get_aca_images

305d458

Fix some DRY and use some CxoTime arithmetic

a2300c1

Fix wrap and use zip

9cbc87a

Some ruff formatting

15c66fa

Fix long line

c48deac

jeanconn mentioned this pull request Nov 6, 2024

spacing of linspace can be more than step_max when supplied sot/cxotime#45

Closed

Simplify

47fd7e4

jeanconn commented Nov 7, 2024

View reviewed changes

Fix test to sort results

e748730

taldcroft mentioned this pull request Nov 7, 2024

Migrate maude_decom to CxoTime #183

Merged

2 tasks

taldcroft self-requested a review November 7, 2024 11:54

taldcroft approved these changes Nov 7, 2024

View reviewed changes

jeanconn merged commit e13497f into master Nov 7, 2024
2 checks passed

jeanconn deleted the maude-aca-chunks branch November 7, 2024 13:41

taldcroft mentioned this pull request Nov 21, 2024

get_aca_images() returns MaskedColumns only for data with masked values (except IMG) #182

Merged

2 tasks

javierggt mentioned this pull request Dec 9, 2024

2024.12 sot/skare3#1450

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use CxoTime.linspace on get_aca_images for > 3hour range #181

Use CxoTime.linspace on get_aca_images for > 3hour range #181

jeanconn commented Oct 17, 2024 •

edited

Loading

taldcroft Oct 29, 2024

taldcroft Oct 29, 2024

jeanconn Nov 6, 2024

taldcroft Oct 29, 2024

taldcroft Oct 29, 2024

jeanconn Nov 7, 2024

taldcroft Nov 7, 2024

jeanconn Nov 7, 2024

taldcroft Nov 7, 2024 •

edited

Loading

jeanconn Nov 7, 2024

-    maude_fetch_times = CxoTime.linspace(start, stop, step_max=3.0 * u.hour)
+    maude_fetch_times = CxoTime.linspace(start, stop, step_max=None)
+    if step_max is None:
+        step_max = MAUDE_FETCH_STEP_MAX

		assert abs(imgs[0]["TIME"] - CxoTime(start).secs) < 2
		assert abs(imgs[-1]["TIME"] - CxoTime(stop).secs) < 2

Use CxoTime.linspace on get_aca_images for > 3hour range #181

Use CxoTime.linspace on get_aca_images for > 3hour range #181

Conversation

jeanconn commented Oct 17, 2024 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taldcroft Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanconn commented Oct 17, 2024 •

edited

Loading

taldcroft Nov 7, 2024 •

edited

Loading