-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix unique over a dimension to use isequal
over ==
#42737
Conversation
For reference, discovered here: https://discourse.julialang.org/t/is-this-a-bug-of-the-julia-function-unique/70141/22 |
|
||
# With NaNs: | ||
E = [1 NaN 3; 1 NaN 3; 1 NaN 3]; | ||
@test isequal(unique(E, dims=1), [1 NaN 3]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs a test for dims=2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And dims=3
, dims=4
..? Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the current implementation it is unlikely to break depending on dimension, but since the observed problem was qualitatively fairly different (adding a column), it seems possible to me that the added test could help in the future (and it's a pretty much free test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dims=1
is the default so we should at least test dims=2
maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the current implementation it is unlikely to break depending on dimension, but since the observed problem was qualitatively fairly different (adding a column)
There was an identical problem for dims=1
where a row was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these needs more tests, so I added merge-me
unique
is documented to useisequal
.Fixes #42733