Skip to content

Conversation

@Moelf
Copy link
Contributor

@Moelf Moelf commented Oct 2, 2020

fix #581

Copy link
Member

@nalimilan nalimilan left a comment

Choose a reason for hiding this comment

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

Thanks!

@Moelf Moelf requested review from mschauer and nalimilan October 3, 2020 15:55
Co-authored-by: Milan Bouchet-Valat <[email protected]>
@Moelf
Copy link
Contributor Author

Moelf commented Oct 3, 2020

@nalimilan isn't the non-copy method defined on 345L for AbstractArray? what's the problem of using collect here

@nalimilan
Copy link
Member

@nalimilan isn't the non-copy method defined on 345L for AbstractArray? what's the problem of using collect here

sort makes a copy, that's why I proposed using sort!.

@Moelf
Copy link
Contributor Author

Moelf commented Oct 3, 2020

@nalimilan done. Last thing, what needs to be changed for addcounts!(cm::Dict{Bool}, x::AbstractArray{Bool}; alg = :ignored) etc. ?

@nalimilan
Copy link
Member

Yes that's would be nice too, as it gives higher performance for these cases.

@Moelf
Copy link
Contributor Author

Moelf commented Oct 3, 2020

sorry, I just don't know what "change" were you referring to. They look like different specialized function to handle different T again.

@nalimilan
Copy link
Member

I mean that they should accept any x of the right eltype, not just AbstractArray. That requires a few dispatch tricks by passing eltype(x) and an argument and dispatching on that.

@Moelf
Copy link
Contributor Author

Moelf commented Oct 3, 2020

maybe we should just collect once at the very beginning at let the rest do their own thing....

because there's no parametric types for iterator and I don't like jamming eltypes everywhere in the function body to manual dispatch

actually, I have an idea let me try.

@Moelf
Copy link
Contributor Author

Moelf commented Oct 3, 2020

@nalimilan how about this?

@Moelf Moelf requested a review from nalimilan October 3, 2020 18:37
Moelf and others added 2 commits October 3, 2020 15:00
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Moelf and others added 5 commits October 3, 2020 15:01
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
@Moelf
Copy link
Contributor Author

Moelf commented Oct 3, 2020

done. Manually adding special case for x that doesn't have length is quite annoying, maybe a re-factor is called....

Moelf and others added 3 commits October 4, 2020 11:03
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
@Moelf Moelf requested a review from nalimilan October 4, 2020 16:11
Moelf and others added 2 commits October 5, 2020 10:21
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Co-authored-by: Milan Bouchet-Valat <[email protected]>
@Moelf Moelf requested a review from nalimilan October 6, 2020 15:28
@nalimilan nalimilan merged commit 591d045 into JuliaStats:master Oct 6, 2020
@nalimilan
Copy link
Member

Thanks @Moelf!

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.

countmap should accept any iterator

3 participants