Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tolerance args to clip_and_warn #161

Merged
merged 2 commits into from
Nov 22, 2023
Merged

Conversation

taldcroft
Copy link
Member

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

  • Mac
(ska3) ➜  chandra_aca git:(allow-grid-model-extrapolation) git rev-parse HEAD
7d6134e5486f3a54ffff8057579ad94a507d4652
(ska3) ➜  chandra_aca git:(allow-grid-model-extrapolation) pytest chandra_aca  
==================================================== test session starts ====================================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 197 items                                                                                                         

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

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
Copy link
Member Author

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

Copy link
Contributor

@javierggt javierggt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with this change. The only comment is that this is the typical change that I will completely forget, and if this warning were to be relevant again I would miss it.

I did the same functional test as in the PR description.

@taldcroft
Copy link
Member Author

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

@taldcroft taldcroft merged commit 6c6c8fa into master Nov 22, 2023
2 checks passed
@taldcroft taldcroft deleted the allow-grid-model-extrapolation branch November 22, 2023 13:05
@jeanconn
Copy link
Contributor

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants