-
Notifications
You must be signed in to change notification settings - Fork 861
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
Optimize distributed model sync #3131
Conversation
X_ref, | ||
y_ref, | ||
X_pseudo_ref, | ||
y_pseudo_ref, |
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.
Is there a reason these aren't just kwargs? Currently if I added some new fit arg like X_unlabeled
, I would need to edit this code (at least I think) for it to work in distributed. Is there a way we can avoid this and make the logic more general?
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.
This function is being shared by distributed and local parallel folding. You would need to edit this function anyway to make it work locally.
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.
ok, how about if I remove the comment about distributed?
Is there a reason these aren't just kwargs? Is there a way we can avoid this and make the logic more general? For example, X_val_ref
isn't present here.
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.
nvm on X_val_ref, I see you use fold
to get that
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.
deep dived the code, I see the reasoning for how it is structured, disregard this comment.
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.
LGTM, added one comment above. (Approval is based on assumption that CI passes)
Job PR-3131-c398834 is done. |
Issue #, if available:
Description of changes:
Example run with some logs enabled:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.