Skip to content

Comments

Refine P2PSync#3588

Merged
longjon merged 1 commit intoBVLC:masterfrom
junshi15:P2psyncPrepare
Mar 6, 2016
Merged

Refine P2PSync#3588
longjon merged 1 commit intoBVLC:masterfrom
junshi15:P2psyncPrepare

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.

}

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