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

With dyn_bgd_n_faint option, override it in the GuideTable too #193

Merged
merged 1 commit into from
Aug 8, 2023

Conversation

jeanconn
Copy link
Contributor

@jeanconn jeanconn commented Jun 29, 2023

Description

Pass the dyn-bgd-n-faint option value through to the GuideTable.

Fixes issue seen in JUL0323A on obsid 25274 that when using sparkles with the --dyn-bgd-n-faint 2 override, acceptable candidates with that option were marked with CRITICAL warnings because the override was not available for the checking of the guide stars.

The override really shouldn't be needed going forward, but I think it should just get in now so if we move to dyn-bgd-n-faint=3 or need to override a value in a pickle file it will be applied correctly.

Interface impacts

Testing

I tested on Linux with /proj/sot/ska3/test set to 2023.3rc9 .

Unit tests

  • Linux

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

Functional test outputs at https://icxc.cfa.harvard.edu/aspect/test_review_outputs/sparkles-pr193/ .

I ran

sparkles /data/mpcrit1/mplogs/2023/JUL0323/oflsa/output/JUL0323A_proseco.pkl.gz --dyn-bgd-n-faint 2 --report-dir flight
export PYTHONPATH=/home/jeanconn/git/sparkles # set to more-dyn branch
python -m sparkles /data/mpcrit1/mplogs/2023/JUL0323/oflsa/output/JUL0323A_proseco.pkl.gz --dyn-bgd-n-faint 2 --report-dir test

The "flight" outputs at https://icxc.cfa.harvard.edu/aspect/test_review_outputs/sparkles-pr193/flight/ show the critical warnings on 25274 and 27940 because the dyn_bgd_n_faint value of 2 was not used in the guide checking.

The "test" outputs at https://icxc.cfa.harvard.edu/aspect/test_review_outputs/sparkles-pr193/test/ show no critical warnings on those obsids (correctly).

Those are the only diffs.

@jeanconn jeanconn changed the title WIP: With dyn_bgd_n_faint option, override it in the GuideTable too With dyn_bgd_n_faint option, override it in the GuideTable too Jul 25, 2023
@jeanconn jeanconn requested a review from taldcroft August 4, 2023 23:23
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.

Testing looks good and the code change is simple.

@jeanconn jeanconn merged commit 9311f3e into master Aug 8, 2023
@jeanconn jeanconn deleted the more-dyn branch August 8, 2023 12:33
@javierggt javierggt mentioned this pull request Sep 6, 2023
@javierggt javierggt mentioned this pull request Sep 18, 2023
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