Skip to content

Remove bad acqs processing and acqs failed N times warnings#456

Merged
jeanconn merged 2 commits intomasterfrom
remove-acq-fails
Aug 21, 2025
Merged

Remove bad acqs processing and acqs failed N times warnings#456
jeanconn merged 2 commits intomasterfrom
remove-acq-fails

Conversation

@jeanconn
Copy link
Copy Markdown
Contributor

@jeanconn jeanconn commented Aug 19, 2025

Description

Remove bad acqs processing and acqs failed N times warnings.

The acquisition failure counts don't include any of our modern understanding of acquisition probability and are unhelpful at this point.

Interface impacts

Testing

Unit tests

  • Mac
(ska3-latest) flame:starcheck jean$ pytest
============================================================================ test session starts =============================================================================
platform darwin -- Python 3.12.8, pytest-8.3.4, pluggy-1.5.0
rootdir: /Users/jean/git
configfile: pytest.ini
plugins: socket-0.7.0, anyio-4.7.0, timeout-2.3.1
collected 14 items                                                                                                                                                           

starcheck/tests/test_state_checks.py .............                                                                                                                     [ 92%]
starcheck/tests/test_utils.py .                                                                                                                                        [100%]

============================================================================= 14 passed in 3.79s =============================================================================
(ska3-latest) flame:starcheck jean$ git rev-parse HEAD
e96999e568dc2ada53056163b638b902b1c75c90

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

I ran the last two weeks as both regression and functional test. The diffs are as-expected with these Bad Acquisition Star warnings removed from the new output and no errors.

https://icxc.cfa.harvard.edu/aspect/test_review_outputs/starcheck/starcheck-pr456/combined_diff.txt

@jeanconn
Copy link
Copy Markdown
Contributor Author

@javierggt and @taldcroft - I think out of discussion yesterday that it made sense to just pull the acquisition N fails warnings (since they are not informed at all by modern probabilities etc). Do you agree? If so, this seems trivial.

@taldcroft
Copy link
Copy Markdown
Member

🎉

@jeanconn jeanconn marked this pull request as ready for review August 19, 2025 15:58
Copy link
Copy Markdown
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.

Awesome!

@jeanconn jeanconn merged commit b08683a into master Aug 21, 2025
2 checks passed
@jeanconn jeanconn deleted the remove-acq-fails branch August 21, 2025 15:46
@javierggt javierggt mentioned this pull request Sep 22, 2025
@javierggt javierggt mentioned this pull request Oct 15, 2025
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