Implement acquisition box overlap penalty

taldcroft commented 8 months ago

Description

This starts from #393 but reduces the scope slightly and does the implementation in a way that tries to factor out the changes to the extent possible.

The overlap penalty consists of multiplying the existing acquisition probability by 0.7 for search boxes that are within 20 arcsec of overlapping and where the penalized star is at least 0.2 mag brighter than the overlapping star. This is derived from statistics for the acquisition success of bright stars that have an overlapping search box with a fainter star. The value of 0.7 is the worst case.

Review

This was reviewed and approved by SS&AWG on 2024-Mar-13.

Interface impacts

No API changes but it will change which stars are selected in cases where an overlapping box would have been chosen.

Testing

Unit tests

[ ] No unit tests
[x] Mac
[ ] Linux

[ ] Windows


(ska3-flight-2024.1rc4) ➜  proseco git:(overlap-penalty-minimal) pytest        
================================================ test session starts =================================================
platform darwin -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0
rootdir: /Users/aldcroft/git
configfile: pytest.ini
plugins: timeout-2.2.0, anyio-4.3.0
collecting ... Intel MKL WARNING: Support of Intel(R) Streaming SIMD Extensions 4.2 (Intel(R) SSE4.2) enabled only processors has been deprecated. Intel oneAPI Math Kernel Library 2025.0 will require Intel(R) Advanced Vector Extensions (Intel(R) AVX) instructions.
Intel MKL WARNING: Support of Intel(R) Streaming SIMD Extensions 4.2 (Intel(R) SSE4.2) enabled only processors has been deprecated. Intel oneAPI Math Kernel Library 2025.0 will require Intel(R) Advanced Vector Extensions (Intel(R) AVX) instructions.
collected 166 items

proseco/tests/test_acq.py ..................................... [ 22%] proseco/tests/test_catalog.py .......................................... [ 47%] proseco/tests/test_core.py ........................... [ 63%] proseco/tests/test_diff.py ...... [ 67%] proseco/tests/test_fid.py ............... [ 76%] proseco/tests/test_guide.py ................................... [ 97%] proseco/tests/test_mon_full_cat.py .... [100%]

================================================ 166 passed in 29.73s ================================================ (ska3-flight-2024.1rc4) ➜ proseco git:(overlap-penalty-minimal) git rev-parse HEAD
6de4daaf628c6f19dd44f7d939997130b0142ae6


Independent check of unit tests by Jean
- [x] Linux speedy test

jeanconn-fido> git rev-parse HEAD 6de4daaf628c6f19dd44f7d939997130b0142ae6 jeanconn-fido> pytest ============================================================= test session starts ============================================================= platform linux -- Python 3.11.8, pytest-7.4.4, pluggy-1.4.0 rootdir: /proj/sot/ska/jeanproj/git configfile: pytest.ini plugins: anyio-4.3.0, timeout-2.2.0 collected 166 items

proseco/tests/test_acq.py ..................................... [ 22%] proseco/tests/test_catalog.py .......................................... [ 47%] proseco/tests/test_core.py ........................... [ 63%] proseco/tests/test_diff.py ...... [ 67%] proseco/tests/test_fid.py ............... [ 76%] proseco/tests/test_guide.py ................................... [ 97%] proseco/tests/test_mon_full_cat.py .... [100%]

============================================================== warnings summary =============================================================== ../../../../ska3/test/lib/python3.11/site-packages/tables/node.py:251: 1 warning proseco/proseco/tests/test_acq.py: 35 warnings proseco/proseco/tests/test_catalog.py: 52 warnings proseco/proseco/tests/test_core.py: 17 warnings proseco/proseco/tests/test_fid.py: 18 warnings proseco/proseco/tests/test_guide.py: 108 warnings proseco/proseco/tests/test_mon_full_cat.py: 5 warnings /proj/sot/ska3/test/lib/python3.11/site-packages/tables/node.py:251: DeprecationWarning: alltrue is deprecated as of NumPy 1.25.0, and will be removed in NumPy 2.0. Please use all instead. self._v_objectid = self._g_open()

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html ================================================ 166 passed, 236 warnings in 116.08s (0:01:56)



### Functional tests
<!-- Describe and document results of any functional tests, otherwise leave the text below -->
See the validation notebooks included in this PR.

jeanconn commented 8 months ago

I note that #393 wasn't really working yet because my penalty was crap. Much improved in your code. I've run into the issue with this PR code that optimization doesn't always seem to succeed in avoiding the overlap with the smaller penalty. So a question about defining what is sufficient and if we want to nudge toward not overlapping based on the penalty determined from your analysis or just prohibit overlap.

This one looks to get me overlap in this PR code for example.

proseco.get_aca_catalog(obsid=29044, exclude_ids_acq=[4860216])

jeanconn commented 8 months ago

I note in passing that it looks like the P2 if you exclude the overlapping star

proseco.get_aca_catalog(obsid=29044, exclude_ids_acq=[4860216, 614602512 ])

looks correct and higher but optimization doesn't seem to arrive at that solution?

taldcroft commented 8 months ago

Thanks, interesting examples. Though I wonder if you have any examples that don't require excluding a star (which never really happens in flight scheduling) to show a problem.

This PR is also an exploration to see if a smaller-footprint strategy can work and maybe it won't. The optimization currently tries to replace the worst p_acq star with a new one after optimizing half-widths. For 29044 it is not possible to shrink the boxes enough to get rid of the overlap.

jeanconn commented 8 months ago

"Though I wonder if you have any examples that don't require excluding a star (which never really happens in flight scheduling) to show a problem." Right -- I was using excluding a star as a way to "apply pressure" to the catalog that I already knew had overlap but haven't made an overlap data set (or used yours from the exploration notebook) for testing. Will take a look.

I also figured that with this PR strategy that optimization tries box sizes and maybe some other stars but doesn't check the case of don't-use-this-star-at-all, which might be what you actually want in some cases.

jeanconn commented 8 months ago

Reviewing with the updated code, it looks to me like only two obsids in the last 6 years (>2018:001), obsids 25281 and 27476 end up with overlap in the optimized boxes (with a larger than 0.2 mag sep), and since you've set the code to update p_acq, once can see the reduced p_acq due to overlap in the final catalog. That looks great to me.

taldcroft commented 8 months ago

I'm slightly concerned about performance since this makes star selection 25% slower. There might be some optimization that can reduce this.

jeanconn commented 8 months ago

One question I didn't ask of this data set was how often this gives allowed-to-overlap boxes within the 0.2 mag sep deadband.

One performance optimization I suppose would be to just not allow overlaps in the box-size-optimization by short-circuiting that instead of going through the full updated-p-acq-and-p-safe calculations.

taldcroft commented 8 months ago

@jeanconn - After the recent commits I think the performance is fine now, it's less than a 5% increase. Do you have ideas for other unit or functional tests?

taldcroft commented 8 months ago

I'm planning to make a ruff PR from master and hopefully rebase this on that. All the lint problems here are silly.

jeanconn commented 8 months ago

Great performance ideas and implementation! (Haven't reviewed in detail yet).

jeanconn commented 8 months ago

Regarding validation and the impact on selection, I assume that https://github.com/sot/proseco/blob/fbf13c0318e237b776c0e6a795aecd809d75a7f7/validate/pr394-overlap-penalty-selection.ipynb is calculating final P2 without the penalty for the "no penalty" part. Not sure if it is worth the effort, but to me, at some level I'm not interested in P2 without the penalty calculated, I'm interested with and without the penalty applied in selection.

taldcroft commented 8 months ago

@jeanconn - this was a point of confusion for me at first, but I believe the results are what you want. The environment variable override is only for star selection via the call to get_aca_catalog. Then later when calc_p_safe is called it does the default behavior of applying the overlap penalty for both catalogs. In the final plot, the red stars for catalogs with overlaps are mostly "better" because the P2 for aca_no_penalty catalog is made worse by the penalty (instead of the aca_penalty catalog being better by some luck of finding better stars).

jeanconn commented 8 months ago

I don't immediately see how calc_p_safe could be using the penalties for P2 when the penalty looks to only be calculated if the env var is not set to true (in get_overlap_penalties) and the boxes are defined as always having zero overlap if the var is set to true. I was thinking that it would make sense just to add that in the notebook by redetermining overlaps for the "penalty not used in selection" cases or by force-including all the stars and boxes as determined by that star selection.

taldcroft commented 8 months ago

@jeanconn - see the latest version of the selection validation notebook. I'm hoping this will make sense to you.

jeanconn commented 8 months ago

And yeah, that helps. The labeling is still challenging, if "p2_no_penalty" is p2 with the penalty included but of the penalty-not-used-in-selection catalog.

taldcroft commented 8 months ago

I updated the notebook to add clarification on the values and interpretations of the plots. Thanks for the feedback, this would probably have been confusing to SSAWG and I'm glad to have thought it through.

jeanconn commented 8 months ago

These changes also include support of the new env var for testing, but I don't see that as an interface impact that needs to be advertised in the description, as it is not intended to be used outside of local-to-proseco testing.

sot / proseco