search

sot / proseco

Probabilistic star evaluation and catalog optimization
https://sot.github.io/proseco
BSD 3-Clause "New" or "Revised" License
1 stars 0 forks source link

Support monitor windows and full catalog #346

Closed taldcroft closed 3 years ago

taldcroft commented 3 years ago

Description

This is to address #305 and make the aca catalog be the complete specification of the star catalog that ORviewer will simply translate into a DOT star catalog.

Testing

Functional testing

from proseco import get_aca_catalog
from proseco.tests.test_common import mod_std_info

row = 10
col = 20
mag = 7.5
monitors = [[row, col, 1, mag, 2],  # Tracking MON
            [row + 350, col + 400, 1, mag, 3],  # Fixed MON
            [1053.38, -275.16, 2, 8.0, 1]  # Guide from MON (force 8x8)
            ]
aca = get_aca_catalog(monitors=monitors, **mod_std_info(n_guide=5, n_fid=1))

aca
<ACATable length=12>
 slot  idx      id    type  sz   p_acq    mag    maxmag   yang     zang    dim   res  halfw
int64 int64   int64   str3 str3 float64 float64 float64 float64  float64  int64 int64 int64
----- ----- --------- ---- ---- ------- ------- ------- -------- -------- ----- ----- -----
    0     1         1  FID  8x8   0.000    7.00    8.00   922.59 -1737.89     1     1    25
    1     2    139192  BOT  6x6   0.981    7.77    9.27   587.27   802.49    28     1   160
    2     3 611190016  BOT  6x6   0.982    6.97    8.47   175.44 -1297.92    28     1   160
    5     4 611192384  GUI  8x8   0.000    8.19    9.69  1053.38  -275.16     1     1    25  <== Guide from MON
    6     5       100  MON  8x8   0.000    7.50    9.00   -19.39    75.30     2     0    25  <== Tracking MON
    7     6       201  MON  8x8   0.000    7.50    9.00 -1751.09  2061.24     7     0    25  <== Fixed MON
    3     7 688523960  ACQ  6x6   0.983    5.93    7.43  -202.71 -1008.91    28     1   160
    4     8 611192384  ACQ  6x6   0.979    8.19    9.69  1053.38  -275.16    28     1   160
    5     9 688521312  ACQ  6x6   0.976    8.45    9.95  -739.68 -1799.85    28     1   160
    6    10 611189488  ACQ  6x6   0.972    8.71   10.21  1536.83  -786.12    28     1   160
    7    11 688522008  ACQ  6x6   0.963    9.04   10.54  -743.78 -2100.94    28     1   160
    0    12    134680  ACQ  6x6   0.964    9.05   10.55  1314.00  1085.73    28     1   160

Fixes #305

cc: @jeanconn @jskrist

taldcroft commented 3 years ago

@jskrist @jeanconn - this PR is nominally complete from the code perspective. I plan to add more tests, but the functional testing is pretty thorough now so I'm hoping the tests won't drive many changes.

jeanconn commented 3 years ago

Is the stuff in validate valuable enough to check-in or does it live in another repo? I'd like to be able to run the validation notebook and fiddle with stuff.

taldcroft commented 3 years ago

Is the stuff in validate valuable enough to check-in

Can you be more specific about the "stuff"? The two notebooks mentioned in the description are intended as a permanent "published" version of the validation. This follows the precedent of the other validation notebooks that were presented for proseco promotion. You can download those two notebooks locally and run them and fiddle.

jeanconn commented 3 years ago

Regarding validate, my mistake. I didn't see the routines in this PR, was working in another directory, had /home/jeanconn/git/proseco in my PYTHONPATH, and forgot that validate/starcheck_proseco.py was from previous validation and also wouldn't import from my PYTHONPATH that way. I suppose I could add /home/jeanconn/git/proseco/validate to my PYTHONPATH to have access if I want to import starcheck_proseco.py without being local or some such.

taldcroft commented 3 years ago

@jeanconn - you have a point about that particular function get_starcheck_obs_kwargs being of use in a broader context. So I moved it into tests.test_common in e5305de. I updated the two validation notebooks accordingly and also took out the /Users/aldcroft bit (but they still assume ~/git/agasc exists and points to something useful).

jeanconn commented 3 years ago

In my initial review, interface and testing/validation looks great.

I'm still a little confused about some of the internals. The redefinition of n_guide as guide + mon has tripped me up a few times (maybe there is another path to be taken there?), as has the fact that plain monitor stars are in cat.guides (I would have expected cat.mons or some such).

jeanconn commented 3 years ago

Oh, and if MON stars stay in guides I think we'll need fixes in other pieces like the guide star reporting, and maybe https://github.com/sot/sparkles/blob/37a56f5bedf7263da777a3ccbffdf581d212eaae/sparkles/core.py#L471 ?

taldcroft commented 3 years ago

Oh, and if MON stars stay in guides I think we'll need fixes in other pieces like the guide star reporting, and maybe https://github.com/sot/sparkles/blob/37a56f5bedf7263da777a3ccbffdf581d212eaae/sparkles/core.py#L471 ?

Good point. I'll look into changing this.

taldcroft commented 3 years ago

About n_guide, I'm not sure there is an easy solution because of the auto-conversion capability. When you call get_aca_catalog you won't know exactly how many guide stars you'll get. The only thing you know is how many slots are allocated for guide + mon. We could rename that to n_guide_mon, but I don't think that is worth the churn. I can certainly update the docs at least to reflect the updated meaning of n_guide.

Also, it may help that with splitting out monitors to a new mons table, the attribute n_guide will correctly reflect the number of guide stars selected. It just won't necessarily match the input n_guide arg, but that was always the case. So maybe all good?

jeanconn commented 3 years ago

Right, I totally understand no easy solution on n_guide as a call_arg. I was initially confused, for example, that even if I specified the MON to not be auto that I still had to set n_guide to be "desired slots for guides + mons" or "most likely 8 - n_fids". But I see that's probably a better convention than having n_guide as a call arg behave differently for auto MON -> guide observations.

So yes, I think if the attribute n_guide matches the number of selected guide stars we should be a little more consistent even if n_guide ends up a bit overloaded.

jeanconn commented 3 years ago

Regarding external API, I don't entirely recall how we tried to handle catalog slot/idx ordering oddities for a BOT star used as a MON when there is also a GUI in the catalog. For example in this constructed example:

obsid = 23378
acas = pickle.load(gzip.open("/data/mpcrit1/mplogs/2020/NOV2320/oflsa/output/NOV2\
320A_proseco.pkl.gz"))
aca = acas[obsid]
call_args = aca.call_args.copy()
call_args['n_guide'] = 5
star = (197.029792,-65.306028,5.530000,'MON')
monitors = [[star[0], star[1], MonCoord.RADEC, star[2], MonFunc.GUIDE]]
call_args['monitors'] = monitors
cat = proseco.get_aca_catalog(**call_args)
acar = cat.get_review_table()
acar.run_aca_review()
<ACATable length=12>
 slot  idx      id     type  sz   p_acq    mag    maxmag   yang     zang    dim   res  halfw
int64 int64   int64    str3 str3 float64 float64 float64 float64  float64  int64 int64 int64
----- ----- ---------- ---- ---- ------- ------- ------- -------- -------- ----- ----- -----
    0     1          2  FID  8x8   0.000    7.00    8.00  -773.20 -1742.03     1     1    25
    1     2          3  FID  8x8   0.000    7.00    8.00    40.01 -1871.10     1     1    25
    2     3          4  FID  8x8   0.000    7.00    8.00  2140.23   166.63     1     1    25
    3     4 1179272040  BOT  6x6   0.972    7.88    9.38  2389.67    86.64    28     1   160
    4     5 1179272560  BOT  6x6   0.977    8.39    9.89  2178.81 -1608.38    28     1   160
    5     6 1179272760  BOT  6x6   0.976    8.49    9.99  1076.66    41.61    28     1   160
    7     7 1179273480  BOT  8x8   0.983    6.12    7.62    69.41    32.79    28     1   160
    6     8 1179403384  GUI  6x6   0.000    8.41    9.91  1452.47 -1660.32     1     1    25
    0     9 1179913456  ACQ  6x6   0.981    7.88    9.38 -1966.52  -597.39    28     1   160
    1    10 1179269944  ACQ  6x6   0.974    8.60   10.10  -339.56  1053.65    28     1   160
    2    11 1179388832  ACQ  6x6   0.973    8.65   10.15  -446.24 -2302.05    28     1   160
    6    12 1179271096  ACQ  6x6   0.969    8.86   10.36  -634.59   496.25    28     1   160

I don't know if the only reason for the previous ordering was to keep some SAUSAGE code happy or if there might be a reason that slots need to show up in a certain order in the actual command (if that is the case, it seems like we'd need to split the slot 7 BOT into a GUI and an ACQ?). We talked about ordering a bit in the aca@ thread "Change in ACA catalog ordering (non-impacting, hopefully)", but I think in the end it is just a matter of testing?

taldcroft commented 3 years ago

@jeanconn - for better or worse, your comments inspired me to totally refactor how the monitor window handling is done. Monitors now look a lot more like acqs, guides, and fids. It is much better I think, but ended up taking most of the day just to get back to passing the same tests. :smile:

taldcroft commented 3 years ago

but I think in the end it is just a matter of testing?

Yup. We'll test and in the unlikely event that something in MATLAB breaks we will revisit (mostly likely by fixing MATLAB, not changing how we do operations). In the end Eric was OK with the changes from the OBC / FOT operations perspective.

jeanconn commented 3 years ago

Thanks for the reorganization to make .mons ! What were your thoughts about disposition for n_guide?

jeanconn commented 3 years ago

I'll have to dig in to the code a bit more; I just realized I'm not clear on which position is being used when the monitor target resolves to a useable guide star. For within 2 arcsecs it doesn't really matter but I confess to not being sure what is proposed.

taldcroft commented 3 years ago

I just realized I'm not clear on which position is being used when the monitor target resolves to a useable guide star. For within 2 arcsecs it doesn't really matter but I confess to not being sure what is proposed.

If the monitor request gets converted to a guide star, then this means the resolved AGASC ID is force-included as a normal guide star, with the exception that it ends up with 8x8 in all cases. The position and mag from the monitor target in the OR list is ignored (observers are not trusted with guide stars).

jeanconn commented 3 years ago

And for the case when an agasc star is identified (and put in the proseco table) but it is left as a MON, the position of the agasc star is ignored?

taldcroft commented 3 years ago

And for the case when an agasc star is identified (and put in the proseco table) but it is left as a MON, the position of the agasc star is ignored?

Correct. It uses the AGASC MAG_ACA but the observer's position. The mag is basically informational, but the rationale is that the user-supplied mag is not in any well-defined mag system, and we we expect in that star catalog column is the best estimate of ACA mag.