Use commands v2 for AGASC supplement update #135

javierggt · 2022-04-12T16:46:12Z

Description

This PR completely removes all uses of timelines and kadi commands v1.

A preliminary exploration of the differences in the two observation lists (from commands v1 and v2) can be found here. We went over that notebook a while ago, and since then kadi commands v2 became the standard, so at this point we trust that the observations are properly defined.

to do:

test_obs_status.py::test_override fails.

Interface impacts

The columns of star_obs_catalogs.STARS_OBS will change (see notebook).

Consequently the columns in mag_stats_obsid.fits file also change. This file can be updated during promotion by running the following script on it:

from astropy import table;
master = table.Table.read('mag_stats_obsid.fits');
master.remove_column('timeline_id');
master.write('mag_stats_obsid.fits', overwrite=True);

Testing

Comprehensive functional test is described below.

Unit tests

Mac

Independent check of unit tests by [REVIEWER NAME]

[PLATFORM]:

Functional tests

starting with an empty directory:

python -m agasc.scripts.update_mag_supplement --start 2021:011 --stop 2021:012

starting with an empty directory. In this case, the --multi-process option failed in master, not in the branch.
```
python -m agasc.scripts.update_mag_supplement --start 2022:001 --stop 2022:090 [--multi-process]
```
This time range includes the latest safe mode. This test failed both in master and the branch when given the --report option. I did not fix it because: a) it might be related to the safe mode, b) it is unrelated to this PR because it failed in both cases.
```
python -m agasc.scripts.update_mag_supplement --start 2023:040 --stop 2023:050
```

The following tests a typical weekly update, where I had to change the mag_stats_obsid.fits file to remove the timeline_id column:

cp $SKA/data/agasc/agasc_supplement.h5 $SKA/data/agasc/rc/*.fits .

only in the branch case, ran this script to remove the timeline_id column from mag_stats_obsid.fits:

from astropy import table;
master = table.Table.read('mag_stats_obsid.fits');
master.remove_column('timeline_id');
master.write('mag_stats_obsid.fits', overwrite=True);

run:

python -m agasc.scripts.update_mag_supplement --stop 2023:085 --report

where the time is a bit after the last update:

import tables
from cxotime import CxoTime
with tables.open_file('agasc_supplement.h5') as h5: print(CxoTime(h5.root.mags[:]['last_obs_time']).max().date)

There were 101, stars out of more than 2440, whose magnitude changed by more than 0.001. I wrote all those AGASC IDs into a file and ran the following
```
 python -m agasc.scripts.update_mag_supplement --agasc-id-file agasc_ids
 python -m agasc.scripts.mag_estimate_report --all-stars --input-dir . --output-dir report --start 2021:001
```
- Reports: master, branch.
- One star-observation gave the No level 0 data in v2 (AGASC 896538696, OBSID 26758)
- The magnitude differences in this table might be different from the previous tests, because now all star-observations for a given star are used (the previous tests used a stop time). Only 96 differ by more than 0.001.
- All differences arise from stars having a different number of observations, and they make sense to me.
- No star magnitude changed more than 0.1.
- There are 30 stars whose magnitude changes by more than 0.01:

 agasc_id  mag_aca_master mag_aca_branch mag_aca_err_master mag_aca_err_branch
---------- -------------- -------------- ------------------ ------------------
  84809168          6.051          6.080              0.031              0.032
 105262824          8.930          8.919              0.037              0.037
 121112928          7.711          7.700              0.031              0.032
 151915504          7.610          7.586              0.031              0.031
 152308032          7.676          7.595              0.034              0.112
 152837536          9.177          9.162              0.035              0.033
 176950392          9.084          9.071              0.033              0.033
 180228464          6.934          6.922              0.031              0.031
 181278800          9.096          9.071              0.035              0.041
 181806456          8.327          8.299              0.032              0.035
 181806584          7.766          7.752              0.031              0.034
 186390704          9.288          9.277              0.035              0.033
 225061880          7.836          7.815              0.031              0.031
 252318288          9.053          9.017              0.039              0.038
 253239472          9.357          9.335              0.036              0.041
 254943440          7.927          7.900              0.031              0.031
 263464240          9.564          9.577              0.039              0.042
 292949928          8.741          8.705              0.032              0.032
 380769128          8.717          8.684              0.040              0.042
 447479952          8.864          8.848              0.034              0.036
 449842032          8.774          8.784              0.032              0.032
 724569152          9.052          9.065              0.039              0.036
 888277056          9.409          9.367              0.032              0.034
 888277248          8.699          8.687              0.031              0.031
 888285704          9.126          9.111              0.033              0.031
 939663504          8.549          8.561              0.031              0.032
 966787648          8.889          8.878              0.038              0.037
1102709168          9.103          9.081              0.032              0.034
1197998392          9.791          9.777              0.051              0.053
1197999176          8.881          8.852              0.032              0.033

jeanconn · 2022-04-20T15:31:53Z

This is just a WIP but it looks like I ran into a little trouble running it on my laptop:

In [1]: from agasc.supplement.magnitudes import star_obs_catalogs
   ...: 

In [2]: stars_obs_v2 = star_obs_catalogs.get_star_observations()
   ...: 

HASH: Out of overflow pages.  Increase page size
---------------------------------------------------------------------------
error                                     Traceback (most recent call last)
<ipython-input-2-b3fe61c90950> in <module>
----> 1 stars_obs_v2 = star_obs_catalogs.get_star_observations()

~/git/agasc/agasc/supplement/magnitudes/star_obs_catalogs.py in get_star_observations(start, stop, obsid)
     23 
     24     with commands.conf.set_temp('commands_version', '2'):
---> 25         catalogs = commands.get_starcats(start=start, stop=stop, obsid=obsid)
     26         observations = Table(commands.get_observations(start=start, stop=stop, obsid=obsid))
     27         for cat in catalogs:

~/git/kadi/kadi/commands/observations.py in get_starcats(start, stop, obsid, scenario, cmds, as_dict)
    365                     starcat = starcat_dict
    366 
--> 367                 starcats_db[db_key] = starcat_dict
    368 
    369             starcats.append(starcat)

~/miniconda3/envs/ska3/lib/python3.8/shelve.py in __setitem__(self, key, value)
    123         p = Pickler(f, self._protocol)
    124         p.dump(value)
--> 125         self.dict[key.encode(self.keyencoding)] = f.getvalue()
    126 
    127     def __delitem__(self, key):

error: cannot add item to database

Is "out of overflow pages" the same as out of memory? This doesn't need to run on my laptop but interesting.

jeanconn · 2022-04-20T15:39:08Z

Though I suppose I'm really not sure if my comment about "out of overflow pages" belongs here or on sot/kadi#221 .

javierggt · 2022-04-20T15:44:11Z

this error comes straight from the kadi function, so it should be there. Still, it sounds like something to consider in the kadi PR.

javierggt · 2022-04-20T15:45:30Z

ah, but not necessarily, because I am not using the _table version of the kadi commands function. I wonder if the error would still happen if I were.

taldcroft · 2022-09-14T14:02:39Z

The bit about the HASH error is now at sot/kadi#252. This shouldn't be considered a blocker on this PR but it will require attention.

taldcroft · 2023-01-23T11:48:02Z

sot/kadi#252 has been closed by sot/kadi#253.

I confirm that the crash that @jeanconn noted no longer occurs. With no cache file I ran the following. It took 4 minutes, but ran without problems. The second time running took 17 seconds.

stars_obs_v2 = star_obs_catalogs.get_star_observations()

I don't understand the design of this module which is reading every star catalog in the entire mission every time, but that is out of scope here.

taldcroft · 2023-01-23T14:43:00Z

agasc/supplement/magnitudes/star_obs_catalogs.py

-STARS_OBS_MAP = None
-STARS_OBS_NP = None
+
+def get_star_observations(start=None, stop=None, obsid=None, join_keys=['starcat_date', 'obsid']):


Is there a reason to have join_keys as a free parameter that can be specified? If this function is really returning a table of unique "star obs" records as defined in the rest of the agasc code, then nothing but join_keys=['starcat_date', 'obsid'] will work.

Related, add a docstring here.

I moved the join keys into the function.

taldcroft · 2023-01-23T14:43:34Z

agasc/supplement/magnitudes/star_obs_catalogs.py

-STARS_OBS_NP = None
+
+def get_star_observations(start=None, stop=None, obsid=None, join_keys=['starcat_date', 'obsid']):
+    from kadi import commands


This import doesn't need to be local.

I moved it out of the function. This import indeed needed to be local when kadi was django based, because of the multi-process option.

taldcroft · 2023-01-23T14:46:36Z

agasc/supplement/magnitudes/star_obs_catalogs.py

+        observations = observations[~observations['starcat_date'].mask]
+        # the following line removes manual commands
+        observations = observations[observations['source'] != 'CMD_EVT']
+        observations['idx'] = np.arange(len(observations))


What is this idx column doing? If it is needed later on then add a comment here about why.

taldcroft · 2023-01-23T14:50:03Z

agasc/supplement/magnitudes/star_obs_catalogs.py

+            _load_startcat_commands(tstop)
+            _load_observed_catalogs()


Useful for testing this PR, but at this point all the commands V1 functionality can be removed.

this was completely removed.

taldcroft · 2023-01-23T14:56:48Z

I ran this branch and ran the testing notebook with a few of my own checks, and I think this is basically ready modulo the minor comments.

… v2 does not get rid of non-normal-pointing times

taldcroft · 2023-04-12T17:59:15Z

Following up on the original sanity check from @jeanconn -

In [1]: from agasc.supplement.magnitudes import star_obs_catalogs
In [2]: stars_obs_v2 = star_obs_catalogs.get_star_observations()
In [4]: stars_obs_v2.info
Out[4]: 
<Table length=229656>
      name       dtype  shape    class    
--------------- ------- ----- ------------
          obsid   int64             Column
         simpos   int64             Column
       obs_stop   str21             Column
    manvr_start   str21             Column
       targ_att float64  (4,)       Column
      npnt_enab    bool             Column
      obs_start   str21             Column
       prev_att float64  (4,)       Column
mp_starcat_time   str21       MaskedColumn
    starcat_idx   int64       MaskedColumn
         source    str8             Column
           slot   int64             Column
            idx   int64             Column
       agasc_id   int64             Column
           type    str3             Column
             sz    str3             Column
            mag float64             Column
         maxmag float64             Column
           yang float64             Column
           zang float64             Column
            dim   int64             Column
            res   int64             Column
          halfw   int64             Column
            row float64             Column
            col float64             Column
    mag_aca_err float64             Column

It works!

taldcroft · 2023-04-12T18:36:39Z

agasc/supplement/magnitudes/star_obs_catalogs.py

-    # Check that the association is correct.
-    dt = CxoTime(dwells['manvr_start'][idxs]) - CxoTime(STARCAT_CMDS['mp_starcat_time'])
-    ok = (dt.sec > 1) & (dt.sec < 1200)
+    with commands.conf.set_temp('commands_version', '2'):


Not needed any more.

taldcroft

Looks good pending fix of that commands V2 conf set command and some sort of sanity check after that.

javierggt force-pushed the commands-v2 branch from 07e2455 to 97ca22d Compare April 25, 2022 16:11

javierggt requested a review from taldcroft April 26, 2022 20:56

taldcroft mentioned this pull request Sep 14, 2022

Problem creating entire starcats.db cache file in one session sot/kadi#252

Closed

javierggt mentioned this pull request Oct 22, 2022

Use kadi commands v1 for now #140

Merged

2 tasks

taldcroft reviewed Jan 23, 2023

View reviewed changes

taldcroft mentioned this pull request Jan 23, 2023

Stop maintaining timelines and commands v1 sot/kadi#230

Closed

22 tasks

taldcroft changed the title ~~WIP: Commands v2~~ Use commands v2 for AGASC supplement update Jan 24, 2023

Changed magnitude supplement to use only commands-v2

23d198f

javierggt force-pushed the commands-v2 branch from 33f46a4 to 23d198f Compare April 5, 2023 21:59

javierggt added 2 commits April 7, 2023 12:26

change mag-stats test data to account for the fact that kadi.commands…

4fff99b

… v2 does not get rid of non-normal-pointing times

do not crash if n_kalman is zero

1564345

javierggt requested a review from taldcroft April 7, 2023 18:32

taldcroft reviewed Apr 12, 2023

View reviewed changes

taldcroft approved these changes Apr 12, 2023

View reviewed changes

remove statement temporarilly setting kadi commands version 2

807e2b7

javierggt merged commit 04eb65d into master Apr 28, 2023

This was referenced Jun 1, 2023

Update sot/agasc to 4.15.0 sot/skare3#1096

Closed

2023.4 sot/skare3#1108

Merged

javierggt mentioned this pull request Jul 12, 2023

2023.3 sot/skare3#1088

Merged

javierggt mentioned this pull request Aug 1, 2023

Release ska3-flight 2023.3 sot/skare3#1089

Closed

41 tasks

jeanconn deleted the commands-v2 branch October 3, 2023 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use commands v2 for AGASC supplement update #135

Use commands v2 for AGASC supplement update #135

javierggt commented Apr 12, 2022 •

edited

Loading

jeanconn commented Apr 20, 2022

jeanconn commented Apr 20, 2022

javierggt commented Apr 20, 2022 •

edited

Loading

javierggt commented Apr 20, 2022

taldcroft commented Sep 14, 2022

taldcroft commented Jan 23, 2023

taldcroft Jan 23, 2023

javierggt Apr 6, 2023

taldcroft Jan 23, 2023

javierggt Apr 6, 2023

taldcroft Jan 23, 2023

javierggt Apr 6, 2023

taldcroft Jan 23, 2023

javierggt Apr 6, 2023

taldcroft commented Jan 23, 2023

taldcroft commented Apr 12, 2023

taldcroft Apr 12, 2023

taldcroft left a comment

Use commands v2 for AGASC supplement update #135

Use commands v2 for AGASC supplement update #135

Conversation

javierggt commented Apr 12, 2022 • edited Loading

Description

Interface impacts

Testing

Unit tests

Functional tests

jeanconn commented Apr 20, 2022

jeanconn commented Apr 20, 2022

javierggt commented Apr 20, 2022 • edited Loading

javierggt commented Apr 20, 2022

taldcroft commented Sep 14, 2022

taldcroft commented Jan 23, 2023

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 commented Jan 23, 2023

taldcroft commented Apr 12, 2023

Choose a reason for hiding this comment

taldcroft left a comment

Choose a reason for hiding this comment

javierggt commented Apr 12, 2022 •

edited

Loading

javierggt commented Apr 20, 2022 •

edited

Loading