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

findprev: use zero(eltype(A)) instead of 0 #18709

Closed
wants to merge 1 commit into from
Closed

findprev: use zero(eltype(A)) instead of 0 #18709

wants to merge 1 commit into from

Conversation

Keno
Copy link
Member

@Keno Keno commented Sep 27, 2016

No description provided.

@Keno Keno changed the title findprev: use zero(eltype(T)) instead of 0 findprev: use zero(eltype(A)) instead of 0 Sep 27, 2016
@StefanKarpinski
Copy link
Member

What was the issue here?

@Keno
Copy link
Member Author

Keno commented Sep 27, 2016

Some types don't implement comparison/promotion with Int64, or if they do, it's not what you want, e.g.

using SIUnits
julia> findlast([0m, 1m, 0m])
3

because

julia> 0m == 0
**false**

(arguable that should have been an error instead)

@@ -1111,7 +1111,7 @@ julia> findprev(A,1)
"""
function findprev(A, start::Integer)
for i = start:-1:1
A[i] != 0 && return i
A[i] != zero(eltype(A)) && return i
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth making this zero(A[i]) in case eltype(A)=Any

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point.

@tkelman tkelman added the needs tests Unit tests are required for this change label Sep 28, 2016
@Keno
Copy link
Member Author

Keno commented Sep 28, 2016

Not sure what happened here. I neither merged nor closed this.

@Keno Keno reopened this Sep 28, 2016
end
return 0
end

"""
findlast(A)

Return the linear index of the last non-zero value in `A` (determined by `A[i]!=0`).
Return the linear index of the last non-zero value in `A` (determined by `A[i]!=zero(A[i])`).
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a genstdlib run

@tkelman tkelman removed the needs tests Unit tests are required for this change label Sep 29, 2016
@simonbyrne
Copy link
Contributor

If we're going to try to support these sort of things it would be good to make a formal specification of the interfaces that need to be supported. As part of my current work I've been creating yet another unitful computation package, and so I've hit a few of these issues as well.

The current pattern is used in quite a few places throughout Base, as there are some cases where it is more efficient (e.g. BigFloat vs Int has an inequality comparison method which doesn't require allocating a new BigFloat)

@tkelman
Copy link
Contributor

tkelman commented Sep 29, 2016

x-ref #15755

@stevengj
Copy link
Member

Seems like we should define an iszero(x) = x == zero(x) function, with specialized versions for bignums and arrays that avoids allocations.

@nalimilan
Copy link
Member

Are we sure we want 0m != 0 and yet iszero(0m) == true? That sounds potentially confusing to me. Types should either compare with other numbers the standard way, or not compare with them at all. Maybe only the definition of find should be changed?

@stevengj
Copy link
Member

@nalimilan. Yes. zero(x) is the additive identity for x. It is not (necessarily) 0. iszero should check whether something is the additive identity. zeros(5,5) == 0 is false, too.

@ararslan
Copy link
Member

ararslan commented Oct 3, 2016

Out of curiosity, why change findprev and not the other find* functions?

@Keno
Copy link
Member Author

Keno commented Sep 29, 2017

Superseded by the find rework stuff.

@Keno Keno closed this Sep 29, 2017
@ararslan ararslan deleted the kf/findprev branch September 29, 2017 20:42
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.

8 participants