-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Small bug in bootstrap keywords #483
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #483 +/- ##
=======================================
Coverage 92.97% 92.98%
=======================================
Files 193 193
Lines 14635 14638 +3
=======================================
+ Hits 13607 13611 +4
+ Misses 1028 1027 -1 ☔ View full report in Codecov by Sentry. |
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.
Hey Alan! Thanks for finding this bug and directly providing a fix.
I have only a minor remark about the test, but other than that, it looks great 🎉
|
||
with pytest.raises(ValueError, match="a must be a positive integer unless no"): | ||
get_moments_cov( | ||
data=data, | ||
calculate_moments=calc_moments, | ||
moment_kwargs=moment_kwargs, | ||
bootstrap_kwargs={"n_draws": -1}, | ||
bootstrap_kwargs={"n_draws": -1, "cluster_by": "cluster"}, |
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.
I would prefer not to add the cluster_by option test to this block. Here we want to check that the bootstrap kwargs are actually passed to the bootstrap function. I think for the cluster_by argument we would rather want to check that an error is thrown if an invalid option is passed. Maybe you could add something like this below:
with pytest.raises(ValueError, match="Invalid bootstrap_kwargs: {'cluster'}"):
get_moments_cov(
data=data,
calculate_moments=calc_moments,
moment_kwargs=moment_kwargs,
bootstrap_kwargs={"cluster": "cluster"},
)
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.
Thanks a lot!
"cluster" is not a bootstrap option