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

dense.dot(sparse) impl #160

Closed
wants to merge 11 commits into from
Closed

dense.dot(sparse) impl #160

wants to merge 11 commits into from

Conversation

pmarks
Copy link
Contributor

@pmarks pmarks commented Apr 29, 2019

This will be allowed on stable once rust-lang/rust#55437 is stabilized. Seem reasonable?

@pmarks pmarks changed the title [DON'T MERGE] sparse.dot(dense) impl [DON'T MERGE] dense.dot(sparse) impl Apr 29, 2019
@vbarrielle
Copy link
Collaborator

That seems mostly reasonable yes, but I'm not sure I like the added genericity on csr_mulac_dense_rowmaj and the likes, I fear it may ask too many type annotations. Maybe it could be
better if Nout were equal to N1 by default.

@pmarks pmarks marked this pull request as ready for review February 22, 2020 22:27
@pmarks pmarks changed the title [DON'T MERGE] dense.dot(sparse) impl dense.dot(sparse) impl Feb 22, 2020
@pmarks
Copy link
Contributor Author

pmarks commented Feb 22, 2020

@vbarrielle this now will compile on stable. It's not possible to have default generic types on methods, so I'm not sure how to reduce the genericity of the csr_mulacc_dense_rowmaj methods.

Any thoughts on what we could do there?

@vbarrielle
Copy link
Collaborator

Hello, sorry for the delayed answer.

I think this added genericity is not too much of a problem, I however need to write a few examples to check if it's actually a breaking change for "typical" calls to csr_mulacc_dense_rowmaj (though I dont think it's called that much alone in the wild).

I hope I'll find the time to do it in the coming week.

vbarrielle added a commit that referenced this pull request Jul 11, 2020
@vbarrielle
Copy link
Collaborator

Hello @pmarks, I've rebased and updated your branch on the latest master, and merged it manually, I'm no longer worried about a possible breaking change as I'm going to bump to 0.8 for other changes as well.

I think I got the gist of your PR right, and tests look ok, but please tell me if I forgot something. After rebasing, I obtained the commits 896c7f2 and 10f42f0

@vbarrielle vbarrielle closed this Jul 11, 2020
@pmarks
Copy link
Contributor Author

pmarks commented Jul 11, 2020

@vbarrielle very cool, thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants