-
Notifications
You must be signed in to change notification settings - Fork 60
[FIX] add n_jobs parameter #962
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
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds an n_jobs parameter to the PermutedOLS IBMA estimator and threads it through to the underlying permuted_ols model fitting call, enabling configurable parallelism instead of a hard-coded single job. Class diagram for updated PermutedOLS IBMA estimatorclassDiagram
class IBMAEstimator
class PermutedOLS {
+bool two_sided
+int n_jobs
+dict parameters_
+__init__(two_sided, n_jobs, kwargs)
+_generate_description()
+_fit_model(beta_maps, n_perm)
}
PermutedOLS --|> IBMAEstimator
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 - I've left some high level feedback:
- Consider adding basic validation or normalization for
n_jobs(e.g., handlingNone,-1, or non-positive values) before passing it through topermuted_olsto avoid surprising runtime errors. - Since
n_jobsis now a first-class estimator parameter, ensure it is consistently surfaced wherever estimator parameters are inspected or serialized (e.g.,get_params/set_paramsor any internal config dicts) so it behaves like the existing options.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding basic validation or normalization for `n_jobs` (e.g., handling `None`, `-1`, or non-positive values) before passing it through to `permuted_ols` to avoid surprising runtime errors.
- Since `n_jobs` is now a first-class estimator parameter, ensure it is consistently surfaced wherever estimator parameters are inspected or serialized (e.g., `get_params`/`set_params` or any internal config dicts) so it behaves like the existing options.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #962 +/- ##
==========================================
+ Coverage 88.26% 88.27% +0.01%
==========================================
Files 49 49
Lines 6926 6994 +68
==========================================
+ Hits 6113 6174 +61
- Misses 813 820 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes # .
Changes proposed in this pull request:
Summary by Sourcery
New Features: