ENH Merge the TaskNode class to the CallbackContext class #11
Conversation
jeremiedbb
left a comment
jeremiedbb
left a comment
There was a problem hiding this comment.
A first round of comments. Implementation looks good. Now that both task nodes and callback context are merged together, we need to unify the vocabulary that we use. There are areas where we talk about task nodes and others where we talk about contexts. I think that we want to keep the notion of task around but we need to clarify somewhere that a callback context object describes the context of a task for instance.
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]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
| i = 0 | ||
| while True: | ||
| subcontext = callback_ctx.subcontext(task_id=i, max_tasks=None) | ||
| subcontext = callback_ctx.subcontext(task_id=i) |
There was a problem hiding this comment.
I'm not sure that None meaning "unknown" is a good idea. Maybe better to be more explicit and use "unknown" ?
Also undecided if it should have a default or should be a required parameter.
There was a problem hiding this comment.
I don't have a strong opinion, but I think max_subtasks=None easily conveys the idea "there is no defined maximum, so you can add any number of subtasks".
jeremiedbb
left a comment
jeremiedbb
left a comment
There was a problem hiding this comment.
Alright let's merge. There are a few places where the doc is unclear between context, task and iter, but I'll iterate on the original PR to unify it properly.
jeremiedbb
merged commit 8a291e5
into
jeremiedbb:base_callbacks_2
2 participants
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Since both the
CallbackContextand theTaskNodeclasses track the same tree structure, the attributes and methods of theTaskNodecan be moved to theCallbackContextto simplify the code structure.A new
_to_dictmethod is added to theCallbackContextto have a dictionary representation of the instance so it can be safely passed to theCallback'son_fit_iter_endandon_fit_endmethods instead of the wholeCallbackContextobject.Any other comments?