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

Reproduction and fixes to the issue #6 #7

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

kywch
Copy link

@kywch kywch commented Aug 28, 2024

Hello, thank you and kudos for the awesome library and paper.

The code below can reproduce issue #6 , which seems to stem from two causes:

def test_no_repeat_random_suggestions(carbs_instance: CARBS) -> None:
    num_to_suggest = 10
    for i in range(num_to_suggest):
        # Simulate a case where the same seed is kept set elsewhere
        carbs_instance._set_seed(1)
        suggestion = carbs_instance._get_random_suggestion()
        observation = ObservationInParam(input=suggestion.suggestion, output=i, cost=i + 1)
        carbs_instance.observe(observation)
        print(suggestion.suggestion)

In my work (and Joseph's pufferlib), we run a seeding function at each training, which also seems to affect CARBS' random sampling. As a result, I got many (like 9) repeated samples out of 10 random samples. The same suggestion is then fed into the observations with different costs, and as Joseph said, "there can be no groups for which the mean is less than the quantile."

My fix addresses both issues:

  • Mask out the param candidates that are in the success and failure observations in _get_mask_for_invalid_points_in_basic(), which is NOT used for resampling.
  • Inspect observations individually, instead of in groups, to create observations_below_min_threshold when the group-based result is empty.

Please let me know if you need anything else.

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.

1 participant