Skip to content

Set star IDs per star and fix get_starcats bug - #352

Merged
taldcroft merged 5 commits into
masterfrom
set-star-ids-per-star
Mar 9, 2026
Merged

taldcroft merged 5 commits into
masterfrom
set-star-ids-per-star

Conversation

@taldcroft

@taldcroft taldcroft commented Apr 4, 2025 •

Copy link
Copy Markdown
Member

Description

This fixes a couple of issues:

  1. When setting the star ID's from a backstop AOSTRCAT command, previously if there was a failed ID then all the subsequent stars were also marked as failed.
  2. The get_starcats function was not correctly using the cmds argument when it was passed. It was only using cmds to get the observations, but then taking the actual starcat commands from the cached global dict of command parameters.

Interface impacts

None.

Testing

Unit tests

  • Mac
(ska3) ➜  kadi git:(set-star-ids-per-star) git rev-parse --short HEAD
a226362
(ska3) ➜  kadi git:(set-star-ids-per-star) pytest
========================================== test session starts ==========================================
platform darwin -- Python 3.13.11, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: anyio-4.12.1, timeout-2.4.0
collected 309 items                                                                                     

kadi/commands/tests/test_commands.py ...........................................................s [ 19%]
..............................                                                                    [ 29%]
kadi/commands/tests/test_commands_v2.py sssssssssssssssssssssssssssssssssssssssssssssssssssssssss [ 47%]
sssssssssssssssssssssssssssssss                                                                   [ 57%]
kadi/commands/tests/test_filter_events.py ..                                                      [ 58%]
kadi/commands/tests/test_states.py ...............................................x.............. [ 78%]
............                                                                                      [ 82%]
kadi/commands/tests/test_validate.py ......................                                       [ 89%]
kadi/tests/test_events.py ..........                                                              [ 92%]
kadi/tests/test_occweb.py .......................                                                 [100%]

======================== 219 passed, 89 skipped, 1 xfailed in 119.00s (0:01:59) =========================

Commands v2

(ska3) ➜  kadi git:(set-star-ids-per-star) env KADI_CMDS_VERSION=2 pytest                               
================================================ test session starts ================================================
platform darwin -- Python 3.13.11, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: anyio-4.12.1, timeout-2.4.0
collected 309 items                                                                                                 

kadi/commands/tests/test_commands.py ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss [ 23%]
ssssssssssssssssss                                                                                            [ 29%]
kadi/commands/tests/test_commands_v2.py ..................................................................... [ 51%]
...................                                                                                           [ 57%]
kadi/commands/tests/test_filter_events.py ..                                                                  [ 58%]
kadi/commands/tests/test_states.py ...............................................x.......................... [ 82%]
                                                                                                              [ 82%]
kadi/commands/tests/test_validate.py ......................                                                   [ 89%]
kadi/tests/test_events.py ..........                                                                          [ 92%]
kadi/tests/test_occweb.py .......................                                                             [100%]

============================== 218 passed, 90 skipped, 1 xfailed in 116.04s (0:01:56) ===============================

Independent check of unit tests by Javier

  • OSX
(ska3-flight) ~/SAO/git/kadi set-star-ids-per-star $ git rev-parse HEAD          
a226362cf60a353aa07e8867d1905ca65220601a
(ska3-flight) ~/SAO/git/kadi set-star-ids-per-star $ export KADI_CMDS_VERSION=3
(ska3-flight) ~/SAO/git/kadi set-star-ids-per-star $ pytest kadi               
============================================================ test session starts =============================================================
platform darwin -- Python 3.13.11, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/javierg/SAO/git
configfile: pytest.ini
plugins: anyio-4.12.1, timeout-2.4.0
collected 309 items                                                                                                                          

kadi/commands/tests/test_commands.py ...........................................................s..............................        [ 29%]
kadi/commands/tests/test_commands_v2.py ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss       [ 57%]
kadi/commands/tests/test_filter_events.py ..                                                                                           [ 58%]
kadi/commands/tests/test_states.py ...............................................x..........................                          [ 82%]
kadi/commands/tests/test_validate.py ......................                                                                            [ 89%]
kadi/tests/test_events.py ..........                                                                                                   [ 92%]
kadi/tests/test_occweb.py .......................                                                                                      [100%]

=========================================== 219 passed, 89 skipped, 1 xfailed in 139.39s (0:02:19) ===========================================
(ska3-flight) ~/SAO/git/kadi set-star-ids-per-star $ export KADI_CMDS_VERSION=2
(ska3-flight) ~/SAO/git/kadi set-star-ids-per-star $ pytest kadi               
============================================================ test session starts =============================================================
platform darwin -- Python 3.13.11, pytest-9.0.2, pluggy-1.6.0
rootdir: /Users/javierg/SAO/git
configfile: pytest.ini
plugins: anyio-4.12.1, timeout-2.4.0
collected 309 items                                                                                                                          

kadi/commands/tests/test_commands.py ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss        [ 29%]
kadi/commands/tests/test_commands_v2.py ........................................................................................       [ 57%]
kadi/commands/tests/test_filter_events.py ..                                                                                           [ 58%]
kadi/commands/tests/test_states.py ...............................................x..........................                          [ 82%]
kadi/commands/tests/test_validate.py ......................                                                                            [ 89%]
kadi/tests/test_events.py ..........                                                                                                   [ 92%]
kadi/tests/test_occweb.py .......................                                                                                      [100%]

=========================================== 218 passed, 90 skipped, 1 xfailed in 132.52s (0:02:12) ===========================================

Functional tests

No functional testing.

So one failure does not abort the rest
@taldcroft
taldcroft force-pushed the set-star-ids-per-star branch from d6f9770 to 1cd3ceb Compare March 8, 2026 20:01
@taldcroft taldcroft changed the title WIP set star IDs per star Set star IDs per star and fix get_starcats bug Mar 9, 2026
)
assert np.count_nonzero(starcats[0]["id"] == -999) == 0
assert np.count_nonzero(starcats[0]["mag"] == -999) == 0
assert np.count_nonzero(starcats[1]["id"] == -999) == 3

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous value of 3 highlights the original bug because only one star in the catalog is not in AGASC 1.8.

start, stop = "2021:365:19:00:00", "2022:002:01:25:00"
"""Test getting star catalogs with commands"""
# The start is the AOSTRCAT date for the first of 7 observations.
start, stop = "2021:365:18:39:25.983", "2022:002:01:25:00"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needed fine tuning because get_observations has an inclusive view of the start/stop range compared to get_cmds which is the exact command date range.

@taldcroft
taldcroft requested review from javierggt and jeanconn March 9, 2026 11:33

@javierggt javierggt left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are good changes. I also think it is good to remove the exception from the logic, not just because it fixes the present issue of skipping subsequent rows. Whenever I have used exceptions to change the flow of the program, I have ended up regretting it.

Similar as PR #381, I think the commands v2 tests might need update. I get these failures:

_________________________________________________ test_get_starcat_only_agasc1p8 _________________________________________________

    def test_get_starcat_only_agasc1p8():
        """For obsids 3829 and 2576, try AGASC 1.8 only
    
        For 3829 star identification should succeed, for 2576 it fails.
        """
        with (
            conf.set_temp("cache_starcats", False),
            conf.set_temp("date_start_agasc1p8", "1994:001"),
        ):
            # Force AGASC 1.8 and show that star identification fails
            with ska_helpers.utils.set_log_level(kadi.logger, "CRITICAL"):
                starcats = get_starcats(
                    "2002:365:16:00:00", "2002:365:19:00:00", scenario="flight"
                )
            assert np.count_nonzero(starcats[0]["id"] == -999) == 0
            assert np.count_nonzero(starcats[0]["mag"] == -999) == 0
>           assert np.count_nonzero(starcats[1]["id"] == -999) == 3
E           AssertionError: assert np.int64(1) == 3
E            +  where np.int64(1) = <function count_nonzero at 0x10362daf0>(<Column name='id' dtype='int64' length=13>\n         1\n         4\n         5\n1180048472\n1181749304\n1211498544\n1211508992\n1211501128\n1181748464\n1181746776\n      -999\n1211502976\n1211507712 == -999)
E            +    where <function count_nonzero at 0x10362daf0> = np.count_nonzero

kadi/commands/tests/test_commands_v2.py:1104: AssertionError
__________________________________________________ test_get_starcats_with_cmds ___________________________________________________

    def test_get_starcats_with_cmds():
        start, stop = "2021:365:19:00:00", "2022:002:01:25:00"
        cmds = commands.get_cmds(start, stop, scenario="flight")
        starcats0 = get_starcats(start, stop)
        starcats1 = get_starcats(cmds=cmds)
>       assert len(starcats0) == len(starcats1)
E       assert 7 == 6
E        +  where 7 = len([<ACATable length=11>\n slot  idx      id    type  sz    mag   ...   yang     zang    dim   res  halfw\nint64 int64   in...468.81  -646.74     1     1    25\n    7     9 125311224  ACQ  8x8    9.14 ...  2308.41   709.62     8     1    60, ...])
E        +  and   6 = len([<ACATable length=10>\n slot  idx      id    type  sz    mag   ...   yang     zang    dim   res  halfw\nint64 int64   in...... -1534.55 -1989.80    16     1   100\n    2    11 29884784  ACQ  6x6    9.64 ...   647.71  -848.31    24     1   140])

kadi/commands/tests/test_commands_v2.py:1113: AssertionError
==================================================== short test summary info =====================================================
FAILED kadi/commands/tests/test_commands_v2.py::test_get_starcat_only_agasc1p8 - AssertionError: assert np.int64(1) == 3
FAILED kadi/commands/tests/test_commands_v2.py::test_get_starcats_with_cmds - assert 7 == 6

@taldcroft

Copy link
Copy Markdown
Member Author

@javierggt - I fixed the v2 tests.

@taldcroft
taldcroft merged commit 23d88f9 into master Mar 9, 2026
5 checks passed
@taldcroft
taldcroft deleted the set-star-ids-per-star branch March 9, 2026 19:29
@javierggt javierggt mentioned this pull request Mar 20, 2026
@javierggt javierggt mentioned this pull request Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants