Skip to content
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

Refine P2PSync #3588

Merged
merged 1 commit into from
Mar 6, 2016
Merged

Refine P2PSync #3588

merged 1 commit into from
Mar 6, 2016

Conversation

junshi15
Copy link

This PR makes the following changes into P2PSync class.

P2PSync::GetInitIter() ... a new getter function for initial iteration number.
P2PSync::run() ... refactored into 2 steps: prepare(), and thread launch/execution
P2PSync::prepare() ... establish the GPU pairs within a process for reduce operations

@shelhamer
Copy link
Member

Please squash the lint fixes -- we like to keep the history clear.

@@ -94,6 +94,9 @@ class P2PSync : public GPUParams<Dtype>, public Solver<Dtype>::Callback,
}

void run(const vector<int>& gpus);
void prepare(const vector<int>& gpus,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be Prepare according to the style guide and existing style. Since we already have run, however, I guess we can postpone that to a future commit.

@longjon
Copy link
Contributor

longjon commented Mar 1, 2016

Thanks, this patch looks good except for the getter name issue above, an optionally fix up existing naming issues (member functions that do nontrivial work should be capitalized CamelCase.)

@junshi15
Copy link
Author

junshi15 commented Mar 4, 2016

Thanks for the review. We have revised the code accordingly.

@junshi15
Copy link
Author

junshi15 commented Mar 4, 2016

It looks there is a problem with travis-CI PYTHON-3.0 builds. It's not a problem with this PR. I will try to build again once that is fixed.

@longjon
Copy link
Contributor

longjon commented Mar 5, 2016

Okay, this looks good except for the (more subtle) point in the comment above; feel free to adjust accordingly and rebase on master which should fix the Travis issue.

@longjon
Copy link
Contributor

longjon commented Mar 6, 2016

Thanks for the patch, the diff and history look clean now, merging.

longjon added a commit that referenced this pull request Mar 6, 2016
@longjon longjon merged commit 54162f8 into BVLC:master Mar 6, 2016
fxbit pushed a commit to Yodigram/caffe that referenced this pull request Sep 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants