sot / chandra_aca

Chandra Aspect Camera Tools
https://sot.github.io/chandra_aca
BSD 2-Clause "Simplified" License
0 stars 0 forks source link

Add tolerance args to clip_and_warn #161

Closed taldcroft closed 9 months ago

taldcroft commented 9 months ago

Description

The current acq probability model only extends to mag=10.75 while proseco will choose stars up to 11.0. This creates many warnings which we routinely ignore.

This PR just accepts that we are currently ignoring those warnings and makes them actually go away. Making a new model that samples up to 11.0 is a fairly substantial undertaking and the time scale for doing it is uncertain. When we do that we can back out this change.

Interface impacts

Removes warnings.

Testing

Unit tests

chandra_aca/tests/test_aca_image.py .............. [ 7%] chandra_aca/tests/test_all.py ........................ [ 19%] chandra_aca/tests/test_attitude.py ............................................................. [ 50%] chandra_aca/tests/test_dark_model.py .... [ 52%] chandra_aca/tests/test_drift.py .......................... [ 65%] chandra_aca/tests/test_maude_decom.py ............... [ 73%] chandra_aca/tests/test_planets.py ............. [ 79%] chandra_aca/tests/test_psf.py ... [ 81%] chandra_aca/tests/test_residuals.py ss... [ 83%] chandra_aca/tests/test_star_probs.py ................................ [100%]

============================================== 195 passed, 2 skipped in 48.26s ==============================================


Independent check of unit tests by [REVIEWER NAME]
- [ ] [PLATFORM]:

### Functional tests
<!-- Describe and document results of any functional tests, otherwise leave the text below -->
I ran the following in ska3, once using all flight packages and once with this PR in PYTHONPATH:

acdc_check_cal ${SKA}/data/mpcrit1/mplogs/2023/NOV1323/oflsa/output/NOV1323A_proseco.pkl.gz 2023:311


In flight this produced a bunch of the clipping warnings, while with the PR there were no warnings. I diffed the text and HTML outputs of the tools and found no diffs.
taldcroft commented 9 months ago

One potential enhancement would be to apply tol_hi=0.25 only for the grid-local-quadratic-2023-05.fits.gz model.

taldcroft commented 9 months ago

@javierggt - Good point and I will probably also forget, but @jeanconn won't! :smile: So I'm going to merge.

jeanconn commented 9 months ago

For this, I still wasn't sure if the best path was to stop warning, or to set probability to 0 (instead of holding on to the 10.75 value for mags fainter than 10.75). Or to set probability to something ad hoc. I think we talked about it overall being low impact but ...

taldcroft commented 9 months ago

At a practical level it doesn't matter. Those mag > 10.75 stars have almost zero impact on P2 so they are not falsely inflating our confidence in a substantial way. But I'd rather have them in the catalog than nothing at all.