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

DM-39178: Add deterministic seed to ks-test model sampling. #191

Merged
merged 1 commit into from May 12, 2023

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented May 11, 2023

No description provided.

Comment on lines +1012 to +1014
# Seed this with the mean value, so that the same data will get the
# same result.
randomSeed = int((mu1 + mu2)/2.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's an interesting thought. But is it really what you want? Upstream changes which cause changes in the inputs will cause a seed-change - why not just hard code it like we do everywhere else? I mean, I think it's fine, just kinda curious if you really do want to do that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if I fully believe in completely hard-coding seeds outside tests; in fact we often use exp number or something that changes but is deterministic. I just wanted something quick that would give the same output for the same input.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I'd forgotten that it was tests I was thinking of. OK, nice, thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The fact the fit is so on the hairy edge that seeds cause it to fail is a different worry though, but for now, fixing random Jenkins failures is clearly more important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the hairy-edge part is going to be fixed on DM-39167.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I figured, thanks 🙂

@erykoff erykoff merged commit a4cd419 into main May 12, 2023
1 check passed
@erykoff erykoff deleted the tickets/DM-39178 branch May 12, 2023 00:00
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.

None yet

2 participants