-
Notifications
You must be signed in to change notification settings - Fork 60
[FIX] fix error when n_iters is being set to None #959
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 GuideAdjusts FWECorrector’s Monte Carlo configuration to avoid overriding estimator defaults when n_iters is None, and adds tests to verify correct parameter propagation and defaults. Class diagram for updated FWECorrector initializationclassDiagram
class FWECorrector {
+str method
+dict parameters
+__init__(method, n_iters, n_cores, kwargs)
}
FWECorrector : method default bonferroni
FWECorrector : n_iters default None
FWECorrector : n_cores default 1
FWECorrector : parameters holds correction configuration
FWECorrector : on init validate method
FWECorrector : on init set parameters based on method and kwargs
FWECorrector : for method montecarlo only set n_iters in parameters when n_iters is not None
FWECorrector : for method montecarlo always set n_cores in parameters
Flow diagram for Monte Carlo FWECorrector parameter handlingflowchart TD
A[start FWECorrector __init__] --> B[check method value]
B -->|method not supported| C[raise ValueError]
B -->|method equals montecarlo| D[handle montecarlo parameters]
B -->|method not montecarlo| G[skip montecarlo config]
D --> E{is n_iters not None?}
E -->|yes| F[set kwargs n_iters to provided n_iters]
E -->|no| H[do not set kwargs n_iters, keep estimator default]
F --> I[set kwargs n_cores to provided n_cores]
H --> I[set kwargs n_cores to provided n_cores]
I --> J[store method in instance attribute]
G --> J
J --> K[store kwargs in parameters attribute]
K --> L[end FWECorrector __init__]
File-Level Changes
Assessment against linked issues
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 there - I've reviewed your changes - here's some feedback:
- Consider applying the same
None-aware logic ton_coresas you did forn_iters(only setting it inkwargswhen notNone) so that estimators can fall back to their ownn_coresdefaults when desired.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider applying the same `None`-aware logic to `n_cores` as you did for `n_iters` (only setting it in `kwargs` when not `None`) so that estimators can fall back to their own `n_cores` defaults when desired.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 #959 +/- ##
=======================================
Coverage 88.25% 88.26%
=======================================
Files 49 49
Lines 6924 6926 +2
=======================================
+ Hits 6111 6113 +2
Misses 813 813 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #922 .
Changes proposed in this pull request:
Summary by Sourcery
Adjust FWECorrector Monte Carlo parameter handling to respect estimator defaults and add regression tests for this behavior.
Bug Fixes:
Tests: