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

Fix bug in halfws for force-include acq stars #325

Closed taldcroft closed 4 years ago

taldcroft commented 4 years ago

Description

This fixes #323 and supercedes #324. The actual bug fix is simple but I spent some time understanding how it can come about and why the original tests did not show a problem. In short the problem comes up when the initial p_acq for included candidate stars does not increase monotonically with decreasing mag. This can happen because of a spoiler or hot pixel.

This only impacts cases with more than one included star. In actual star selection with MATLAB choosing more than one star is not common and typical practice now would be to not explicitly select the halfwidths. It has a definite impact in starcheck which uses force-include of all stars and halfwidths for every catalog.

Testing

Also ran this test from #324 and confirmed that the update changes this test from fail to pass, but did not include in the final PR.

def test_obsid_47250():
    """Test include all 8 acq ids has right halfws for right stars"""

    # This uses the attitude/stars from obsid 47250 in APR2720
    att = [0.37484371, -0.40057847, -0.79053942, 0.27216999]
    stars = StarsTable.from_agasc(att, date='2018:230')
    dark = DARK40.copy()
    kwargs = mod_std_info(att=att, stars=stars, dark=dark,
                          n_guide=8, n_fid=0, n_acq=8, man_angle=90)
    kwargs['include_halfws_acq'] = [140, 160, 160, 160, 160, 160, 160, 160]
    kwargs['include_ids_acq'] = [810423632,
                                 810428640,
                                 812125920,
                                 812130960,
                                 812131816,
                                 886441184,
                                 810427792,
                                 810428880]
    aca = get_aca_catalog(**kwargs)

    # Confirm that the included stars have the assigned halfws
    for id, halfw in zip(kwargs['include_ids_acq'],
                         kwargs['include_halfws_acq']):
        assert aca.acqs.get_id(id)['halfw'] == halfw

Fixes #323 Closes #324

jeanconn commented 4 years ago

The other impact being that, since starcheck force includes all the stars and halfwidths for review, that catalogs in that final review could have incorrect acquisition probabilities due to incorrect internal assignment of box sizes for that assessment.

We'll want to get this promoted on the HEAD side relatively quickly for that use case, but I don't see a big operational impact with proseco 4.8.0 on the FOT side. However, I also don't see work-around from ORViewer if we actually did need to specify halfwidths for stars that have candidates out of mag order.

taldcroft commented 4 years ago

Agreed. Note that your first impact is technically covered by what I said (multiple stars being force-included), but I edited to call out your point that this does regularly happen.

As mentioned I believe that in current practice the case of specifying multiple stars with explicit halfwidths is no longer going to happen in FOT planning. But in any case this bugfix will be available and it now looks like a smaller MATLAB release is going to happen before the 2019b update.