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

Raise informative error for exponentiation to negative power (see #3024) #8784

Merged
merged 1 commit into from
Oct 24, 2014

Conversation

timholy
Copy link
Member

@timholy timholy commented Oct 23, 2014

No description provided.

@ivarne
Copy link
Member

ivarne commented Oct 23, 2014

@JuliaBackports :)

@nalimilan
Copy link
Member

Is there any chance the message could be made even more explicit? Less skilled users are the ones which are going to find this behavior unexpected, and for them I think the message you added is quite difficult to understand. Technically, would it be possible to print something like "use 2.0^-2" when the user typed 2^-2 at the command line?

@timholy
Copy link
Member Author

timholy commented Oct 23, 2014

This doesn't have access to the values themselves, and of course if it was a function with x^n then substituting in the values wouldn't really help change the code.

But I agree with the overall concern. Would

To take an integer x to a negative power -n, try 1/x^n, float(x)^-n, or (x//1)^-n.

be an improvement?

@nalimilan
Copy link
Member

This doesn't have access to the values themselves, and of course if it was a function with x^n then substituting in the values wouldn't really help change the code.

Yeah, I was suspecting that. Your new proposal looks like an improvement to me, but maybe it would be even better to mention somewhere the fact that writing 2.0 instead of 2 also works. Not sure how to express that in all generality. Maybe this?

Cannot raise an integer x to a negative power -n. Make x a float by adding
a zero decimal (e.g. 2.0^-n instead of 2^-n), or write 1/x^n, float(x)^-n, or (x//1)^-n.

@timholy
Copy link
Member Author

timholy commented Oct 23, 2014

Seems like a fine suggestion to me. I'll let this sit a bit and see if anyone else chimes in with further suggestions.

@simonbyrne
Copy link
Contributor

Why couldn't this go in the error itself, similar DimensionMistmatch?

@timholy
Copy link
Member Author

timholy commented Oct 23, 2014

Can't remember for sure, but I think it has to do with GC frames and/or inlining.

@nalimilan
Copy link
Member

Yeah, that's really unfortunate, as explicit errors everywhere make a language much nicer to user, especially for newcomers (and they are the ones that make your user base grow...).

@StefanKarpinski
Copy link
Member

It may be possible at some point to read values straight from the stack instead of forcing them to be heap-allocated, which would allow access to values without a performance hit.

@timholy
Copy link
Member Author

timholy commented Oct 24, 2014

Adopted @nalimilan's suggested wording.

I will be away for a couple days; if this turns green and anyone is in a hurry, just hit merge. Or just wait until I get back.

Also, I only belatedly noticed that this (and the similar handling of other DomainErrors, like sqrt(-4)) doesn't work if the error is inside a function---it's REPL-only. That is indeed a little unfortunate.

StefanKarpinski added a commit that referenced this pull request Oct 24, 2014
Raise informative error for exponentiation to negative power (see #3024)
@StefanKarpinski StefanKarpinski merged commit 48dbe3a into master Oct 24, 2014
@StefanKarpinski
Copy link
Member

For some reason I don't see this even in the REPL on OS X. I just get the normal old DomainError:

julia> 2^-10
ERROR: DomainError
 in power_by_squaring at /Users/stefan/projects/julia/usr/lib/julia/sys.dylib
 in ^ at intfuncs.jl:104
 in process_options at ./client.jl:221
 in _start at ./client.jl:362
 in _start at /Users/stefan/projects/julia/usr/lib/julia/sys.dylib

ivarne added a commit that referenced this pull request Oct 24, 2014
@ivarne
Copy link
Member

ivarne commented Oct 24, 2014

The decoded bt cames back as

(symbol("???"),symbol("???"),4555450214,true,4555450214)
(symbol("???"),symbol("???"),4555450377,true,4555450377)
(:power_by_squaring,symbol("/Users/ivarne/dev/julia/usr/lib/julia/sys.dylib"),-1,false,4588770674)

So I tried with f05660e and it seems to work. Any reason @timholy didn't attach this to the power_by_squaring function?

@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2014

@ivarne I think he's going to be offline for a few days. Guessing he may have just assumed the DomainError was coming directly from a method of the ^ operator and forgot about power_by_squaring? Though it looks like power_by_squaring might throw a DomainError for some non-integer x in x^y where the message would be inaccurate. We can try your change and see whether it causes any false alarms?

Edit: nevermind, on Linux where Tim presumably tested this, his version works and yours doesn't. Some platform-dependent inlining or something going on?

@ivarne
Copy link
Member

ivarne commented Oct 25, 2014

I noticed, but you happened to have a reasonable guess also 😄, and he will be back . power_by_squaring is only for Integer exponents (?), so I don't think it is really misleading.

@tkelman
Copy link
Contributor

tkelman commented Oct 25, 2014

Sorry for my compulsive post-editing, if you haven't hit refresh. Your method works on Mac, Tim's method works on Linux. Dunno why it's different. Check for either case and issue the same message?

@JeffBezanson JeffBezanson deleted the teh/expneg branch October 25, 2014 17:16
@timholy
Copy link
Member Author

timholy commented Oct 26, 2014

Maybe there's another email I haven't yet gotten to about a fix for this, but this looks like it's not due to a difference in dispatch, it's due to a difference in backtraces. Someone on a Mac will probably have to fix & test. (Preferably, someone with access to both Mac & Linux.)

@stevengj
Copy link
Member

stevengj commented Nov 4, 2014

I suppose we could just do || code[1] == :power_by_squaring to handle both cases.

stevengj added a commit that referenced this pull request Nov 4, 2014
timholy added a commit that referenced this pull request Nov 7, 2014
(cherry picked from commit 79c9dfc)
make #8784 work on Mac
(cherry picked from commit d2252d0)
@ivarne
Copy link
Member

ivarne commented Nov 7, 2014

Backported for both Linux and Mac in 201c916

waTeim pushed a commit to waTeim/julia that referenced this pull request Nov 23, 2014
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.

7 participants