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

Require minimum number of anchor stars for dyn bgd bonus #174

Merged
merged 1 commit into from
Jul 6, 2022

Conversation

taldcroft
Copy link
Member

Description

When adding a temperature delta for the dynamic background bonus, require that at least 3 stars are so-called "anchor stars" with no bonus applied. These anchor stars are evaluated as for the legacy background with no bonus.

Interface impacts

None

Testing

Unit tests

  • Mac

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

No functional testing.

# situation where 4 stars are selected and 2 are faint bonus stars. In this case
# there would be only 2 anchor stars that ensure good tracking even without
# dyn bgd.
MIN_DYN_BGD_ANCHOR_STARS = 3
Copy link
Contributor

@jeanconn jeanconn Jun 20, 2022

Choose a reason for hiding this comment

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

This makes sense, but somewhat defeats having dyn_bgd_n_faint be option-driven. But if you go with n_guide - dyn_bgd_n_faint there are separate struggles with the corner cases. Maybe it could be n_guide - dyn_bgd_n_faint with a critical if n_guide != 5 and dyn_bgd_n_faint > 0.

Copy link
Contributor

@jeanconn jeanconn left a comment

Choose a reason for hiding this comment

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

This is fine and useful as as a layer of protection if we want to evaluate the catalog allowing some stars to be checked with dyn_bgd_n_faint but 5 stars were not selected.

@taldcroft taldcroft merged commit e5e3a1f into master Jul 6, 2022
@taldcroft taldcroft deleted the dyn-bgd-min-anchor-stars branch July 6, 2022 19:41
@javierggt javierggt mentioned this pull request Aug 10, 2022
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