-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remove seed argument #431
Remove seed argument #431
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few small comments :) @gowerc
Co-authored-by: Isaac Gravestock <[email protected]> Signed-off-by: Craig Gower-Page <[email protected]>
@gravesti - TY those all look reasonable so have applied them all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good @gowerc
@gravesti - The only thing on my mind is whether the warning for using the seed argument should be upgraded to an error ? As in currently I've completely removed the functionality so using the |
Yeah, it's a bit in the middle now. One way would have been to throw a warning but still set the seed. However now that you're removed the functionality, I agree it might be better to go straight to error, since not setting the seed properly in analysis code will create worse problems. |
@gravesti - Ok upgraded to an error - Please can you reapprove ? |
Closes #415