improve deprecation warning for non-integer indexing

[stevengj]

It seems that 99% of the real-world usages of floating-point indexes (deprecated in #10458) arise because people are using / for integer division when they really want ÷, but the latter is not very intuitive or discoverable.

See also the julia-users discussion.

This simply includes a suggestion to replace i/j with i÷j in the deprecation warning. Probably should be backported to 0.4.

[tkelman]

This strikes me as though it would be actively confusing any time you get this warning from something unrelated to division.

[stevengj]

@tkelman, it does say "if you are doing division" ... is there a way to make that more obvious? Maybe "It may be that your non-integer index came from ...."?

The thing is, in discussions of this topic there have been literally zero real-world examples of non-integer to_index calls that didn't arise from division, so it seems like this should be the bulk of our concern.

[stevengj]

@tkelman, I've attempted to clarify the warning.

[tkelman]

It's just a really long warning to always give for this - you could have almost anything in an indexing expression. Without actually looking at the expression at the parser level this seems inaccurate at best.

[stevengj]

@tkelman, I'm not sure why the length is a concern?

You could have almost anything in theory, but in practice you don't. Do you have any counter-examples from real applications? In contrast, integer division is extremely common in indexing calculations (for strided data, divide-and-conquer algorithms, etc.).

[tkelman]

If length were never a concern, we could print the entire contents of the manual on every warning or error message...

Though I believe most packages have moved away from this due to the deprecation, ApproxFun and a few other places had been overloading getfield with floating-point inputs for function approximation or interpolation.

[stevengj]

@tkelman, we are talking about two sentences here. Hyperbole about printing the whole manual is not helpful. The question is, why is the length of two sentences a concern?

The ApproxFun case and similar interpolation software that overloads getfield is not relevant, because such applications would never call to_index. We are only concerned here with cases where integer-value indices are required (to_index throws an exception for a non integer-valued Real), i.e. traditional arrays and equivalents thereof.

[tkelman]

The question is, why is the length of two sentences a concern?

If it's needing to take 3 lines at default terminal widths, it's probably too long for a warning? Especially since this warning can come from trying to do something as simple as A[1.0].

[stevengj]

We have other warnings and messages that are a couple lines long, e.g the exception message for sqrt(-1). As long as it is short enough to be quickly readable (as opposed to overwhelming the reader with paragraphs of text), I don't see the problem.

And the point is that people are very unlikely to type A[1.0], whereas they are likely to type A[n/2], and this message is helpful. New users do not know about div, and the warning about non-Integer indexes leaves people frustrated and confused — they don't know what to do about n/2.

The question is not whether something is too long or too short, but rather whether shorter or longer would be more helpful. Spitting out pages-long warnings are not helpful because they are too long to quickly digest, especially if you have a lot of them. But just saying non-Integer indices are deprecated is too short, because it gives few clues about how to fix the problem in common cases.

[yuyichao]

I also like this new message. I've personally used Int(n / 2) for a long time before I finally discovered div (and later ÷).

[tkelman]

I'm not objecting all that strongly, just doesn't seem like this is the most effective or accurate place to convey the information that div exists. equivalent to round(Int, i/j, RoundToZero) is also a bit superfluous here - if the user doesn't know what div does prior to seeing this message, they can look it up and easily find out. Just consider using i÷j or div(i,j) instead of division of the form i/j in indexing would likely be enough to get the message across.

[yuyichao]

I think you are generally right that this is only a very specific use case of div and integer division is also only one of the ways you could get a floating point index. However, I think for this particular case, this is indeed how most user is likely going to see this warning so informing the user of the correct way to do it is useful here, as long as it's clear to the user that this message is only relavant for them if they are indeed getting a floating point index from integer division.

This is also not matlab user specific, I tried n // 2 as what I would do in python3 but that apparently doesn't help....

The equivalant to ... might be a little bit unnecessary but it's also not that long...

edit: For the python3 case, it doesn't inform the user about the right way to do integer truncated division in the indexing error message (not a warning) but it is less of an issue for them since the breakage from python2 to python3 is the division and not indexing so the users usually already know what they are looking for (2to3 might handle this too?).

[stevengj]

(It's also useful for people coming from C, where / is div. Also Ruby.)

It's not like we are limited to only talking about div at one place — we can talk about it both here and in the manual and at other places. But since most users are likely to encounter the to_index warning because of /, it is good tell them about div here, regardless of where else we explain it.

[stevengj]

Got rid of the "equivalent to round(...)", I agree that this could be dropped without losing much.