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

Change dyn_bgd_n_faint default to 2 and fix faint force-include bug #403

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Oct 31, 2024

Description

Change the proseco dyn_bgd_n_faint default to 2.

This had originally been left at 0 for continuity and regression testing convenience / historical compatibility, but at this point it is just confusing that the default isn't the "2" that is being used in FOT operations.

Also fixes a bug that force-included guide stars could be discarded if faint.

Interface impacts

Testing

Unit tests

  • Mac
(ska3-flight-latest) flame:proseco jean$ git rev-parse HEAD
0db6f1db75d727aef533c47f20bc559a02b5c663
(ska3-flight-latest) flame:proseco jean$ pytest
========================================================= test session starts ==========================================================
platform darwin -- Python 3.11.8, pytest-8.0.2, pluggy-1.4.0
PyQt5 5.15.9 -- Qt runtime 5.15.8 -- Qt compiled 5.15.8
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: astropy-0.11.0, qt-4.4.0, cov-5.0.0, timeout-2.2.0, remotedata-0.4.1, anyio-4.3.0, filter-subpackage-0.2.0, doctestplus-1.2.1, astropy-header-0.2.2, hypothesis-6.112.0, arraydiff-0.6.1, mock-3.14.0
collected 168 items                                                                                                                    

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

========================================================= 168 passed in 23.14s

Independent check of unit tests by @taldcroft:

  • Mac

Functional tests

test_guides_include_close is a functional test that faint force-included guide stars are included in the final catalog.

@jeanconn jeanconn changed the title Change dyn_bgd_n_faint default to 2 Change dyn_bgd_n_faint default to 2 and fix faint force-include bug Oct 31, 2024
@taldcroft
Copy link
Member

test_guides_include_close is a functional test that faint force-included guide stars are included in the final catalog.

@jeanconn - can you confirm that this test fails without 19d8dab? If so then this looks good.

@jeanconn
Copy link
Contributor Author

Yes. I coded the change for the default to dyn_bgd_n_faint=2 first and then this test started failing here :

assert np.all(np.in1d(include_ids, cat2["id"]))
. So I can confirm if dyn_bgd_n_faint = 2 and the fix included in this PR for drop_excess_bonus_stars is not included that the test fails.

@jeanconn
Copy link
Contributor Author

I can obviously separate the changes if that helps for review and traceability.

Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@jeanconn jeanconn merged commit 23db6ec into master Nov 1, 2024
2 checks passed
@jeanconn jeanconn deleted the dyn-bgd-n-faint-default branch November 1, 2024 12:30
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.

2 participants