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

(±1)^-n should not throw a DomainError #8900

Closed
wants to merge 1 commit into from

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Nov 4, 2014

Since this has an integer result, no need to throw an exception.

@ivarne
Copy link
Member

ivarne commented Nov 4, 2014

What does @JeffBezanson think about the hidden copy(x) in this case? Is it (un)expected as it apparently was with fill?

@stevengj
Copy link
Member Author

stevengj commented Nov 4, 2014

@ivarne, I used copy(x) because that is what power_by_squaring already does in the p == 1 case; I don't have strong opinions about it, but both cases should clearly be consistent.

@stevengj
Copy link
Member Author

stevengj commented Nov 4, 2014

The travis failure looks unrelated to me.

@JeffBezanson
Copy link
Member

It doesn't bother me. First, I'm not sure copy(x) would ever do anything when x==1 is true. Perhaps it should use x == one(x) instead?

Second, I don't think anything is expected about the aliasing behavior of x^p. It would be quite crazy to rely on that returning the same x object in some cases. In contrast, it's reasonable to assume that fill(x, n) reuses the same x object, because (1) there are legitimate uses for arrays of many references to the same object, (2) fill is a structural kind of operation that doesn't transform x.

However, calling copy in ^ would ideally be unnecessary. Those who use mutation need to be careful, not everybody else. But for this function it really doesn't matter.

@stevengj
Copy link
Member Author

stevengj commented Nov 5, 2014

@JeffBezanson, changed to x == one(x).

@@ -77,6 +77,8 @@ function power_by_squaring(x, p::Integer)
elseif p == 2
return x*x
elseif p < 0
x == one(x) && return copy(x)
x == -1 && return iseven(p) ? one(x) : copy(x)
Copy link
Member

Choose a reason for hiding this comment

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

Comparing x == 1 should suffice here. If you want to make the comparison is between like types, it may make sense to construct u = one(x) only once and then also return that instance since this is faster for numeric types that do actually require allocation (e.g. BigInt, BigFloat, etc.). However, it seems like not constructing this object at all would be better in such cases.

@ivarne
Copy link
Member

ivarne commented Nov 5, 2014

When would there be a difference between x == 1 and x == one(x), apart from the efficiency loss associated with initializing a new object?

It might be the case that typeof(x) hasn't defined integer comparisons, but it seems quite dubious to me to have one(::X), but not ==(::X, ::Int).

@StefanKarpinski
Copy link
Member

There shouldn't be a difference but of course people can define these functions to do whatever they want.

@JeffBezanson
Copy link
Member

This function was originally written to also handle matrices, where one(x) is quite different.

@StefanKarpinski
Copy link
Member

Ah, yes. Good point. In that case you definitely don't want to construct it twice.

@stevengj
Copy link
Member Author

stevengj commented Nov 5, 2014

Note that this function is never actually called for matrices when the power is negative, as far as I can tell. (And for general matrices, a DomainError is the wrong fallback in any case.). But I will modify it to call one only once just to be on the safe side.

@ivarne
Copy link
Member

ivarne commented Nov 5, 2014

Just in case anyone else is as confused as me.

one() is defined as multiplicative identity, so for arrays it is the identity matrix.

@stevengj
Copy link
Member Author

stevengj commented Nov 5, 2014

I'm less concerned about matrices (which follow a different code branch for negative powers) than I am about other user-defined fields where one(T) != 1.

@JeffBezanson
Copy link
Member

Can be rebased and merged I think.

@stevengj
Copy link
Member Author

rebased

@ararslan
Copy link
Member

Superceded by #18342?

@tkelman tkelman closed this Sep 16, 2016
@stevengj stevengj deleted the onepower branch September 16, 2016 16:17
@stevengj
Copy link
Member Author

Ah right, I forgot I had already filed a PR for this.

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.

6 participants