Skip to content

Set seed in test_learned_preference_objective to stop it from flaking #2145

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Set seed in test_learned_preference_objective to stop it from flaking #2145

wants to merge 1 commit into from

Conversation

esantorella
Copy link
Member

@esantorella esantorella commented Dec 9, 2023 •
edited
Loading

Motivation

  • The test was flaky due to a varying amount of numerical error depending on random inputs, so I set a seed to a random number between 0 and 10
  • Changed some data to double precision to avoid a warning

Test Plan

  • checked that the test passes for each seed between 0 and 10
  • I confirmed that there are seeds that do cause it to fail
  • Increased the number of samples a lot to confirm that numerical error because small when the number of samples is large -- in other words, the error is due to a low number of samples

@esantorella esantorella requested a review from Balandat December 9, 2023 00:44
@esantorella esantorella self-assigned this Dec 9, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 9, 2023
@facebook-github-bot
Copy link
Contributor

@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Balandat
Balandat approved these changes Dec 9, 2023
View reviewed changes
Copy link
Contributor

@Balandat Balandat left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

@facebook-github-bot
Copy link
Contributor

@esantorella has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@esantorella merged this pull request in 9354fd7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Balandat Balandat Balandat approved these changes

Assignees

@esantorella esantorella

Labels
CLA Signed Do not delete this pull request or issue due to inactivity. Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@esantorella @facebook-github-bot @Balandat