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

iszero vs x==0 and countnz, find etc. #23005

Closed
andreasnoack opened this issue Jul 28, 2017 · 12 comments
Closed

iszero vs x==0 and countnz, find etc. #23005

andreasnoack opened this issue Jul 28, 2017 · 12 comments
Assignees
Milestone

Comments

@andreasnoack
Copy link
Member

andreasnoack commented Jul 28, 2017

In short

julia> "Julia" == 0
false

but

julia> iszero("Julia")
ERROR: MethodError: no method matching zero(::String)
Closest candidates are:
  zero(::Type{Base.LibGit2.GitHash}) at libgit2/oid.jl:106
  zero(::Type{Base.Pkg.Resolve.VersionWeights.VWPreBuildItem}) at pkg/resolve/versionweight.jl:82
  zero(::Type{Base.Pkg.Resolve.VersionWeights.VWPreBuild}) at pkg/resolve/versionweight.jl:124
  ...
Stacktrace:
 [1] iszero(::String) at ./number.jl:22

But String is relatively arbitrarily chosen here. An implication of this is that

julia> countnz([:Julia])
1

julia> countnz([[0.0]])
1

julia> !iszero([0.0])
false

because countnz tests with x != 0. It could also test with !iszero but then countnz([:Julia]) would fail.

This came up in #22945 but is kind of separate from the PR so I think it deserves its own issue. There was a previous discussion of this issue in #17623 (comment) but I'm not sure how many people noticed.

Possible solutions I can come up with are

  1. Define fallback iszero(x) = false and use !iszero whenever testing for zero in e.g. countnz and find. This would make countnz([[0.0]]) == 0 and countnz([:Julia]) == 1
  2. Remove the fallback == method or make it try to convert the arguments to the same type and use !iszero in countnz and find. This would make countnz([[0.0]]) == 0 and countnz([:julia]) fail, but probably also make a million other things throw.
  3. Leave iszero and == as they are but use !iszero in countnz and find. This would make countnz([[0.0]]) == 0 and countnz([:julia]) fail.
  4. Don't do anything. This would make countnz([[0.0]]) == 1 (which I think is the wrong result) and countnz([:Julia]) == 1

Update: I think we should add this to the milestone for 1.0.

@andreasnoack andreasnoack added the needs decision A decision on this change is needed label Jul 28, 2017
@andreasnoack
Copy link
Member Author

I've updated the top post with possible solutions

@JeffBezanson
Copy link
Member

JeffBezanson commented Jul 28, 2017

I guess the root of the problem is that x == 0 is ambiguous: it could be asking whether x is very much like the object 0, or it could be asking whether x is the zero element of its group. For == I think we have no choice but to pick the first interpretation. So I guess I favor option (3): make all functions that explicitly name "zero" or "z" use the group-element interpretation, and leave == alone.

@JeffBezanson JeffBezanson added this to the 1.0 milestone Jul 28, 2017
@tkelman
Copy link
Contributor

tkelman commented Jul 28, 2017

I think == 0 is a better fallback than false (maybe only tried when zero is not applicable?) but otherwise 1 seems like the more desirable behavior to me - if there is no zero element, the answer is no, why error? I anticipate people would immediately start defining zero(::String) in response to the error they would get here, and I don't think we should encourage that

@JeffBezanson
Copy link
Member

Or perhaps iszero(x) = (x == zero(x))? I'm fine with getting an error from countnz(["str"]).

@simonbyrne
Copy link
Contributor

isn't that what it is now?

@JeffBezanson
Copy link
Member

+1 to the idea of making countnz call iszero though.

@vtjnash
Copy link
Member

vtjnash commented Aug 3, 2017

I've only just started reading the issue, but the status quo actual seems best to me.
I think the iszero usage can be easily written:

count(iszero, [1, 2, 3, 0])

@Keno
Copy link
Member

Keno commented Aug 3, 2017

Along the lines of what @vtjnash is proposing, I propose getting rid of the countnz function entirely in favor of count(!iszero, A) or whatever your favorite predicate is.

@StefanKarpinski
Copy link
Member

I think you mean count(!iszero, [1, 2, 3, 0]), right?

@JeffBezanson
Copy link
Member

Removing countnz entirely is ok with me too. I like deleting stuff :)

@StefanKarpinski
Copy link
Member

Efficient implementation for sparse is roughly nnz(S)*pred(default) + count(p, S.values).

@JeffBezanson
Copy link
Member

Fixed by #23485.

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

No branches or pull requests

7 participants