Support `get_observations()` and `get_starcats()` from backstop commands

taldcroft commented 8 months ago

Description

This PR adds the capability to use the get_observations() and get_starcats() functions with commands that have been read using kadi.commands.read_backstop().

Those two get_* functions rely on LOAD_EVENT tlmsid=OBS commands in the CommandTable. In the kadi commands archive those commands are added in the ingest process, but when reading directly from a backstop file (or using a source of commands outside of the kadi archive), then the commands won't include those OBS commands and get_observations() will return an empty list.

The biggest part of the PR is a new function add_observations_to_cmds(), which adds OBS commands to the suppled CommandTable. Most of the work is done with the existing (and very-well tested) kadi.commands.commands_v2.add_obs_cmds() function, but in this context we need to explicitly worry about continuity and have an argument to set the source column to the load name.

The read_backstop() function gets a new argument add_observations=False, which if True will call add_observations_to_cmds() appropriately. There is some logic to infer the load_name from the backstop file path unless a new load_name is provided.

Most commonly users would call cmds = read_backstop(backstop_path, add_observations=True). But the intent is that they could also do cmds = add_observations_to_cmds(cmds, load_name) for any CommandTable.

In order to support continuity for user-supplied commands, two new continuity variables for SIM position and Obsid were added to the core ingest functions in commands_v2.py. This is all done in a back-compatible way so that the archive ingest process is unaffected. (This is unit tested, but I still like to be conservative here).

The rest of the changes are minor, mostly doc strings or formatting.

The function ska_load_dir() is replaced with parse_cm.paths.load_dir_from_load_name().

Interface impacts

Calling kadi.commands.read_backstop() with an existing backstop table issues a FutureWarning that in the future this will raise an exception. Instead use kadi.commands.get_cmds_from_backstop().

Testing

Unit tests

[x] Mac


(masters) ➜  kadi git:(observations-from-cmds) git rev-parse HEAD        
45bf175485a225d33f3942607c81f72e60a65120
(masters) ➜  kadi git:(observations-from-cmds) pytest
==================================================================== test session starts ====================================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 230 items

kadi/commands/tests/test_commands.py ...................................................................................... [ 37%] kadi/commands/tests/test_states.py ......................x.............................................x....................... [ 77%] kadi/commands/tests/test_validate.py .................... [ 86%] kadi/tests/test_events.py .......... [ 90%] kadi/tests/test_occweb.py ...................... [100%]

========================================================= 228 passed, 2 xfailed in 99.82s (0:01:39) =========================================================


Independent check of unit tests by Jean
- [x] Linux (ska3-masters) -- I thought the "git archive" warning was fixed by separate work but maybe that doesn't apply to direct run of tests with pytest.

(ska3-masters) jeanconn-fido> git rev-parse HEAD 45bf175485a225d33f3942607c81f72e60a65120 (ska3-masters) jeanconn-fido> pytest =========================================================================== test session starts ============================================================================ platform linux -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0 rootdir: /proj/sot/ska/jeanproj/git, configfile: pytest.ini plugins: timeout-2.1.0, anyio-3.6.2 collected 230 items

kadi/commands/tests/test_commands.py ...................................................................................... [ 37%] kadi/commands/tests/test_states.py ......................x.............................................x....................... [ 77%] kadi/commands/tests/test_validate.py .................... [ 86%] kadi/tests/test_events.py .......... [ 90%] kadi/tests/test_occweb.py ...................... [100%]

============================================================================= warnings summary ============================================================================= kadi/kadi/commands/tests/test_commands.py::test_get_starcats_each_year[year0] /fido.real/conda/envs/ska3-masters/lib/python3.10/site-packages/setuptools_scm/git.py:295: UserWarning: git archive did not support describe output warnings.warn("git archive did not support describe output")

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ========================================================== 228 passed, 2 xfailed, 1 warning in 198.86s (0:03:18



### Functional tests
<!-- Describe and document results of any functional tests, otherwise leave the text below -->
No functional testing.

jeanconn commented 5 months ago

I think this needs a test that hits prev_obsid and prev_simpos.

taldcroft commented 5 months ago

I think this needs a test that hits prev_obsid and prev_simpos.

Calling add_obs_cmds with those arguments set is currently exercised in the test of the DEC1123A loads, where the code gets the continuity and does this:

    cmds_obs = kcc2.add_obs_cmds(
        cmds,
        kcc2.PARS_DICT,
        kcc2.REV_PARS_DICT,
        prev_att=prev_att,
        prev_simpos=cont["simpos"],
        prev_obsid=cont["obsid"],
    )

For DEC1123A there is no commanding of the SIM (SIM_TRANS) prior to the observation in the test obsid 43907, so seeing the -99616 from continuity is confirmation for simpos_prev. For obsid the obsid is commanded so doing an explicit test would require getting the commands and stripping out the obsid command.

Maybe it's not worth the effort?

jeanconn commented 5 months ago

I was thinking that it wouldn't be much to set prev_simpos and prev_obsid to even something bogus to just confirm they did something.

taldcroft commented 5 months ago

I was thinking that it wouldn't be much to set prev_simpos and prev_obsid to even something bogus to just confirm they did something.

prev_simpos is already tested and confirmed via the existing test as noted. For prev_obsid it is difficult to confirm that it does something because the value is overridden by the obsid commanding in the loads. Thus the need to strip out the obsid commanding in order to see the value you put in. I can do this if you think there is a credible chance it is not working, but basically this will never happen in production usage or even normal testing.

taldcroft commented 5 months ago

I did just discover a real problem with the code this morning which I have yet to diagnose so stand by on further review.

taldcroft commented 5 months ago

Ready again for review.

taldcroft commented 5 months ago

Dang, merge conflict from #312. I forgot the rule to never just add tests to the end of a test file, instead put them in random places. I'm going to hold off on any further rebasing until further review on this.

taldcroft commented 5 months ago

@jeanconn - I think I have addressed all your comments.

sot / kadi