FEA Callbacks base infrastructure + progress bars (Alternative to #27663) #28760
Conversation
adrinjalali
left a comment
adrinjalali
left a comment
There was a problem hiding this comment.
Levels of abstraction, for third party devs at this point we do __sklearn_method_name__ pattern, if they're supposed to implement / override them to modify behavior.
I don't think we should use _method_name much, on things which are called outside the class itself (as in, really treat them as private). But to be able to suggest alternatives, I'd need to better understand this part of the codebase.
Could you maybe add a README file to the callback folder, explaining conceptually the overview of what each object / file is supposed to do? That makes it easier for me to review this, and understand where the scoping challenges are.
|
|
||
| # attach callbacks to the new estimator | ||
| if hasattr(estimator, "_skl_callbacks"): | ||
| new_object._skl_callbacks = clone(estimator._skl_callbacks, safe=False) |
There was a problem hiding this comment.
I think it makes sense to have a test for equivalence of _skl_callbacks on objects in estimator_checks.py to make sure as we keep developing, things stay consistent, including third parties. I don't mind if that happens in a separate PR, before we merge into main
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
5 participants
Alternative to #27663 based on feedback from the drafting meeting. I'm keeping both open for now for easier comparison.