Skip to content

Conversation

@FrancoisPgm
Copy link

Implement the private __skl_fit__ method for the estimators used for the callbacks.

@github-actions
Copy link

github-actions bot commented Sep 30, 2025

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 59bddb5. Link to the linter CI: here

def fit(self, X_train=None, y_train=None, X_val=None, y_val=None):
if isinstance(self, CallbackSupportMixin):
callback_ctx = self.init_callback_context()
callback_ctx.eval_on_fit_begin(estimator=self)
Copy link
Owner

Choose a reason for hiding this comment

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

on_fit_begin should be called in __skl_fit__ imo.

BaseEstimator class.
"""

@_fit_context(prefer_skip_nested_validation=False)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's test what it looks like to extract the content of the decorator and put it inside fit.

Comment on lines 84 to 86
):
for i in range(self.max_iter):
subcontext = callback_ctx.subcontext(task_id=i)
if callback_ctx is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

We need a new method, like set_task_info, to set max_subtasks, task_name and task_id here, now that init_callback_context has moved.

callback_ctx.set_task_info(...)

Comment on lines 83 to 86
self, X_train=None, y_train=None, X_val=None, y_val=None, callback_ctx=None
):
for i in range(self.max_iter):
subcontext = callback_ctx.subcontext(task_id=i)
if callback_ctx is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

callback context cannot be None since we intialize it in fit and pass it to __skl_fit__.

Copy link
Author

Choose a reason for hiding this comment

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

good point

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.

2 participants