Skip to content

Conversation

pevnak
Copy link
Contributor

@pevnak pevnak commented Jun 28, 2021

[Please delete this text and describe your change here.
For bugfixes, please detail the bug and include a test case which your patch fixes.
If you are adding a new feature, please clearly describe the design, its rationale, the possible alternatives considered.
It is easiest to merge new features when there is clear precedent in other systems; we need to know we're taking
the right direction since it can be hard to change later.]

This fixes a bug
#1596
by adding reduce(::typeof(hcat), Vector{TV}) where {TV<:OneHotLike}

PR Checklist

  • [x ] Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable
  • API changes require approval from a committer (different from the author, if applicable)

@pevnak pevnak closed this Jun 28, 2021
@pevnak
Copy link
Contributor Author

pevnak commented Jun 28, 2021

I have not noticed Matej fixed it here
#1595

@darsnack
Copy link
Member

Actually the reduce(hcat, ...) case was removed from that PR before merging. The fix that we agreed upon was to define similar and setindex! for OneHotArrays (which should handle the reduce and other fast paths automatically).

There is an implementation of similar that I wrote in the comments for #1595 which will work. I never got around to setindex!, but a PR with those two changes would be appreciated.

@tantheta01
Copy link

@darsnack wanted to check if I could work on this issue or has a PR been created already?

@ToucheSir
Copy link
Member

AFAIK nobody is working on it, but fair warning that the setindex! piece may be a little tricky!

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.

4 participants