Implement commands archive v2.0 (beta release)

taldcroft commented 2 years ago

Description

This PR implements the command archive v2.0 concept.

This requires https://github.com/sot/parse_cm/pull/35.

Testing

[x] Passes all unit tests on MacOS @ 246ceb9
[x] Functional testing

Interface impacts

New pprint_like_backstop method on CommandTable

V1 commands (currently the default)

Add a time column to the CommandTable returned by get_cmds

V2 commands (beta)

Remove the timeline_id column and add a source column to the CommandTable.
Access network resources (Google sheets, OCCweb) by default
Add LOAD_EVENT pseudo-commands containing observation information
Add get_observations and get_starcats functions.
CommandTable string columns like date and source are kept in encoded ASCII bytes in the table instead of being converted to unicode for improved performance. The astropy Table "unicode-sandwich" machinery should make this largely transparent to the user.

Functional testing

Star and fid identification [OK]

Confirm that with 1.5 arcsec star search box and 40 arcsec fid search box that stars and fids are identified for every observation in the archive. It appears there are a handful of failed IDs for fids before 2008. I suspect these are defects in the commands archive itself, but in any case this low-level of problems for old observations is OK.

def test_get_starcats_each_year(year):
    starcats = get_starcats(start=f'{year}:001', stop=f'{year+1}:001')
    assert len(starcats) > 2
    for starcat in starcats:
        # Make sure fids and stars are all ID'd
        ok = (starcat['type'] != 'MON') & (starcat['type'] != 'FID')
        if np.any(starcat['id'][ok] == -999):
            print('Bad stars', starcat.obsid)
        ok = starcat['type'] == 'FID'
        if np.any(starcat['id'][ok] == -999):
            print('Bad fids', starcat.obsid, starcat.sim_offset, starcat.detector)

for year in range(2003, 2023):
    print(f'{year=}')
    test_get_starcats_each_year(year)

year=2003
Bad fids 3778 -1177 ACIS-I
Bad fids 4000 -1177 ACIS-I
Bad fids 4912 -4 HRC-S
year=2004
Bad fids 4937 -4 HRC-S
Bad fids 60116 -4 HRC-S
year=2005
Bad fids 5587 1 HRC-I
year=2006
Bad fids 6723 -1 ACIS-I
Bad fids 6432 -4 HRC-S
Bad fids 58647 -4 HRC-S
year=2007
Bad fids 7763 -2444 ACIS-S
Bad fids 7525 -2444 ACIS-S
year=2008
year=2009
year=2010
year=2011
year=2012
year=2013
year=2014
year=2015
year=2016
year=2017
year=2018
year=2019
year=2020
year=2021
year=2022

Task schedule [OK]

On Mac with this PR installed to a test Ska3 env:

$ task_schedule3.pl -config kadi/task_schedule_cmds.cfg

This ran successfully and updated $SKA/data/kadi/cmds2.{h5,pkl} as expected.

Configuration [OK]

$ rm ~/.kadi/config/kadi.cfg
$ ipython
In [1]: from kadi import create_config_file
   ...: from kadi.commands import conf
   ...: print(f'{conf.default_lookback=}')
   ...: create_config_file(overwrite=True)
conf.default_lookback=30
INFO: The configuration file has been successfully written to /Users/aldcroft/.kadi/config/kadi.cfg [astropy.config.configuration]
(ska3) ➜  kadi git:(commands-archive) head ~/.kadi/config/kadi.cfg 
[commands]

## Default lookback for previous approved loads (days).
# default_lookback = 30

## Cache backstop downloads in the astropy cache. Should typically be False,
## but useful during development to avoid re-downloading backstops.
# cache_loads_in_astropy_cache = True

## Clean backstop loads (like APR1421B.pkl.gz) in the loads directory that are

Now edit the config file to set default_lookback = 60 and re-run but with overwrite=False:

In [1]: from kadi import create_config_file
   ...: from kadi.commands import conf
   ...: print(f'{conf.default_lookback=}')
   ...: create_config_file(overwrite=False)
conf.default_lookback=60
WARNING: The configuration file already exists and seems to have been customized, so it has not been updated. Use overwrite=True if you really want to update it. [astropy.config.configuration]
Out[1]: False

Tentative migration plan

See #216

Fixes #172

jskrist commented 2 years ago

What do you think about using sheet id unstead of spreadsheet id for scenarios? That way there could be one spreadsheet used for an anomaly response, and different sheets for each scenario being considered (with one central document holding everything).

see Google Sheets API Doc for information about the differences.

taldcroft commented 2 years ago

@jskrist - About sheet id, I'm a little worried about managing permissions. Right now the idea is that the "flight" Commands Sheet is editable by only a small set of people, but viewable by the world. I think if you make another sheet (tab) then it will have the same limited edit permissions, but the idea is that anyone can make their own scenario sheet and edit.

jskrist commented 2 years ago

@taldcroft, that makes sense. I see the power and flexibility of allowing individuals to make scenarios in their own spreadsheets as they are doing analysis, but it puts back a little of the "where is the latest data?" problem, that this central sheet was resolving.

taldcroft commented 2 years ago

@jeanconn - any progress on review of this PR?

jeanconn commented 2 years ago

Not really, I'll have more for today.

taldcroft commented 2 years ago

For review, the new commands core.py module started life as kadi/commands/commands.py, so you can see the diffs somewhat better with:

git show master:kadi/commands/commands.py > commands_master.py
opendiff commands-master.py kadi/commands/core.py

taldcroft commented 2 years ago

@jeanconn - I added a more complete functional test of the star and fid identification in the top description.

jeanconn commented 2 years ago

Looking at the diff for core / commands, what were your thoughts on this bit

    # Convert 'date' from bytestring to unicode. This allows
    # date2secs(out['date']) to work and will generally reduce weird problems.
    out.convert_bytestring_to_unicode()

No longer necessary? It just wasn't clear to me from the original comment of "weird problems" what I should look for in testing to make sure it isn't a problem going forward.

jeanconn commented 2 years ago

Should there be a test that hits the new stuff in parse_cm or are the unit tests over there sufficient?

taldcroft commented 2 years ago

Looking at the diff for core / commands, what were your thoughts on this bit

Great catch, that is an undesired API change for V1. I'll fix it. I also see how to fix Chandra.Time.date2secs so the original weirdness doesn't show up (done in https://github.com/sot/Chandra.Time/pull/53).

jeanconn commented 2 years ago

Regarding "I'll also"... did you decide the fix in Chandra.Time was sufficient so we don't need to bring back the bytestring_to_unicode?

jeanconn commented 2 years ago

Me, I'm OK with the unicode fix being only in Chandra.Time.date2secs and not keeping the convert_bytestring_to_unicode in the V1 code. I'm also OK with no tests in here that directly hit the new parse_cm.csd code. So I think the testing is sufficient for V1 and the testing and docs are sufficient for V2, so I'm going to mark this approved.

taldcroft commented 2 years ago

Thanks for the review @jeanconn ! I actually had deaa502 in my local version for a couple days but forgot to push it. My goal was to make V1 as close to the original as possible. Right now adding the time column is the only difference I know about, and that it very unlikely to break anything.

Testing the custom scenario with an RTS is a good idea. I just adapted the example in the narrative docs to a unit test in 30073a8, so thanks for the push.

sot / kadi