Skip to content

Speedup sparse init #6899

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.

Sign up for GitHub

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

Jump to bottom
Merged
fmassa merged 7 commits into from
May 3, 2018
Merged

Speedup sparse init #6899

fmassa merged 7 commits into from
May 3, 2018

Conversation

@mttk
Copy link
Contributor

@mttk mttk commented Apr 24, 2018 •
edited
Loading

As per #6021

  • Removes the nested for loop in favor of list indexing
  • Obtains ~10-20x speedup on average.

Following the discussion in the issue, I ran a shootout between the currently implemented initialization, the version proposed by @stefanonardo (Opt1) and the version proposed by @fmassa (Opt2).

I ran sparse init on tensors of sizes [(500, 500), (100, 100), (50, 50), (10, 10)] and for sparsities of [1, 0.1, 0.01]. Each initialization was re-ran 10 times. What I learned:

  • The original version is consistently >= 10x slower
  • Opt1 is faster for larger numbers of zero elements (due to what I assume, topk having complexity of k*n), where k is the number of zeros
  • Opt2 is faster for smaller numbers of zero elements

However, in the largest case I tested (500x500 tensor, sparsity=1), Opt2 is about the same speed as the original version, and in the other edge case (500x500 tensor, sparsity=0.01), it is ~27% slower with a higher speed deviation.

Implementation credit goes to @stefanonardo

Full (badly formatted) results are here for the next month: https://pastebin.com/fB4mHqM9

@mttk mttk force-pushed the speedup_sparse_init branch from 9890049 to 7e5ebb6 Compare April 24, 2018 14:32
fmassa
fmassa approved these changes Apr 24, 2018
View reviewed changes
Copy link
Member

@fmassa fmassa left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks!

I have a minor comment, but apart from that this is good to merge!

torch/nn/init.py Outdated
tensor[row_idx, col_idx] = 0

t = tensor[:, col_idx]
t[zero_indices] = 0

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/nn/init.py
for col_idx in range(cols):
row_indices = list(range(rows))
random.shuffle(row_indices)
row_indices = torch.randperm(rows)

This comment was marked as off-topic.

Sign in to view

@fmassa
Copy link
Member

fmassa commented Apr 29, 2018

It looks like there are some tests failing

17:41:49 ======================================================================
17:41:49 FAIL: test_sparse_default_std (test_nn.TestNNInit)
17:41:49 ----------------------------------------------------------------------
17:41:49 Traceback (most recent call last):
17:41:49   File "test_nn.py", line 5124, in test_sparse_default_std
17:41:49     assert column[column == 0].nelement() >= math.ceil(sparsity * cols)
17:41:49 AssertionError
17:41:49 
17:41:49 ----------------------------------------------------------------------

By looking at the failing test, I think that there is a bug in the sparsity check, and it should be math.ceil(sparsity * rows) instead, as in the sparse implementation

Could you verify that this is indeed the case, and fix the test?

@fmassa fmassa added awaiting response (this tag is deprecated) This tag is deprecated while we figure out what to do with it pytorch labels Apr 29, 2018
@ssnl
Copy link
Collaborator

ssnl commented May 1, 2018

@mttk Thanks for your PR. Could you kindly fix the test please?

@mttk
Copy link
Contributor Author

mttk commented May 1, 2018 •
edited
Loading

@ssnl @fmassa sorry -- had an extended weekend so I used it to AFK a bit. Fixed as Francisco suggested, waiting for checks.

Ok -- the int() is necessary. I'm not sure why the original test fails (which states that too few elements are zeros). It's also weird that previously, the tests passed.
I'll install py2.7 pytorch and run tests locally. I'll finish this tomorrow.

@mttk mttk force-pushed the speedup_sparse_init branch from ced6c05 to 5cb4de7 Compare May 3, 2018 10:31
fmassa
fmassa reviewed May 3, 2018
View reviewed changes
test/test_nn.py Outdated
for col_idx in range(input_tensor.size(1)):
column = input_tensor[:, col_idx]
assert column[column == 0].nelement() >= math.ceil(sparsity * cols)
assert column[column == 0].nelement() >= math.ceil(sparsity * cols), "{} : {}, {}".format(column[column == 0].nelement(), sparsity, cols)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@mttk
Copy link
Contributor Author

mttk commented May 3, 2018

@ssnl @fmassa everything works now, thanks Francisco.

@fmassa fmassa merged commit c96f262 into pytorch:master May 3, 2018
@fmassa
Copy link
Member

fmassa commented May 3, 2018

Thanks a lot Martin!

@fmassa fmassa mentioned this pull request May 5, 2018
Closed
Jorghi12 pushed a commit to wsttiger/pytorch that referenced this pull request May 10, 2018
@mttk @Jorghi12
Speedup sparse init (pytorch#6899)
8be8410 
* Sparse initialization speedup

* +empty line

* simplify indexing

* Can't reproduce locally...

* Can't reproduce locally...+

* Can't reproduce locally...+

* Fix test, cleanup
weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018
@mttk @weiyangfb
Speedup sparse init (pytorch#6899)
855e422 
* Sparse initialization speedup

* +empty line

* simplify indexing

* Can't reproduce locally...

* Can't reproduce locally...+

* Can't reproduce locally...+

* Fix test, cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fmassa fmassa fmassa approved these changes

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

@zdevito zdevito Awaiting requested review from zdevito

+2 more reviewers

@stefanonardo stefanonardo stefanonardo left review comments

@apaszke apaszke apaszke approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting response (this tag is deprecated) This tag is deprecated while we figure out what to do with it open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mttk @fmassa @ssnl @apaszke @stefanonardo @ezyang