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

multiplication of {Transpose, Adjoint} of Array and OneHotVector #1424

Merged
merged 10 commits into from
Jun 15, 2021

Conversation

gxyd
Copy link
Contributor

@gxyd gxyd commented Dec 13, 2020

Also fixes #777 . Tests have been added.

src/onehot.jl Outdated Show resolved Hide resolved
src/onehot.jl Outdated Show resolved Hide resolved
src/onehot.jl Outdated Show resolved Hide resolved
test/onehot.jl Outdated Show resolved Hide resolved
@gxyd gxyd changed the title multiplication of OneHotVector and Array multiplication of {Transpose, Adjoint} of Array and OneHotVector Dec 16, 2020
@gxyd
Copy link
Contributor Author

gxyd commented Dec 18, 2020

@mcabbott this is ready for a review.

DhairyaLGandhi
DhairyaLGandhi previously approved these changes Dec 31, 2020
src/onehot.jl Outdated Show resolved Hide resolved
src/onehot.jl Outdated Show resolved Hide resolved
src/onehot.jl Outdated Show resolved Hide resolved
@DhairyaLGandhi
Copy link
Member

Looks good, thanks!

@CarloLucibello
Copy link
Member

I think this is superseded by #1447. We should make sure that the tests added here get in there. Could merge this now, then #1447 should rebase and fix conflicts

@CarloLucibello
Copy link
Member

Or we could wait for #1447 to be merged, then rebase this, and fix #777 if it is not fixed already

@gxyd
Copy link
Contributor Author

gxyd commented Jan 1, 2021

Or we could wait for #1447 to be merged, then rebase this, and fix #777 if it is not fixed already

@CarloLucibello I think this solution sounds better, since that is a new way of implementation for the OneHot. I definitely don't mind if need be the code from this PR directly copied to the new OneHot implementation PR, so we directly fix it in that PR itself

@gxyd
Copy link
Contributor Author

gxyd commented Jan 1, 2021

I'll keep this PR open? Since in case wearen't able to have new one-hot implementation ready for v0.12 , we can probably just think of this PR may be.

@DhairyaLGandhi
Copy link
Member

Probably easier to merge this now since I'd like the tests in and rushing #1447 makes me uncomfortable since it makes strong decisions around using internal Julia functions and complicates its design

@darsnack
Copy link
Member

darsnack commented Feb 7, 2021

Was this addressed by #1448 that superseded #1447? If so, we can close. Actually, looking more closely, I don't think #1447 or #1448 address the issue here? They both still only define *(::AbstractMatrix, ::OneHotArray) instead of the specific type wrappers in this PR.

@ToucheSir
Copy link
Member

Bump? The new OneHotArray has been in a few stable releases now.

@darsnack
Copy link
Member

@gxyd I hope you don't mind, I just rebased your PR (all the code is same and your authorship is preserved). I think this PR is good to go once the checks pass.

@gxyd
Copy link
Contributor Author

gxyd commented Jun 15, 2021

@darsnack I'm glad the code written could be of some help.

Copy link
Member

@darsnack darsnack left a comment

Choose a reason for hiding this comment

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

Thanks @gxyd!

@darsnack
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 15, 2021

Build succeeded:

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.

onehot ambiguous method
6 participants