Raise on out of bounds range access

[billylanchantin]

Fixes: #1005

In the example from that issue we now get:

iex> df = Explorer.DataFrame.new(a: [1, 2, 3])
iex> df[1..1]
# ** (ArgumentError) range 1..1 is out of bounds for a dataframe with 1 column(s)
#     (explorer 0.11.0-dev) lib/explorer/shared.ex:221: Explorer.Shared.to_existing_columns/3
#     (explorer 0.11.0-dev) lib/explorer/data_frame.ex:387: Explorer.DataFrame.fetch/2
#     (elixir 1.18.1) lib/access.ex:322: Access.get/3
#     iex:3: (file)

[billylanchantin]

The bounds check on ranges was a little fiddly since Enum.slice/2 accepts negative bounds. I think I got it but I'm not 100% sure.

[josevalim]

Because ranges can be reverse ordered, I am not sure how much we want to rely on sort. My suggestion would be to validate first and last independently:

for pos <- [first, last] do
  cond do
    pos >= 0 and pos < n_cols -> :ok
    pos < 0 and pos >= -n_cols -> :ok
    true -> raise "..."
  end
end

WDYT?

[billylanchantin]

Enum.slice/2 says it doesn't support reverse ordered ranges yet. Do you want to support them here in anticipation of their future support in Elixir 2.0?

[josevalim]

Ah, good point. If the code above works (big if), then it should be simpler and more future proof. Worth giving it a try?

[billylanchantin]

Sure! Unfortunately though, I think your algorithm doesn't work for the following situation:

df = DF.new(a: [1, 2, 3], b: ["a", "b", "c"], c: [4.0, 5.1, 6.2])

n_cols = DF.n_columns(df) #=> 3
slice = 1..10//10

assert slice.last > n_cols
assert DF.to_columns(df[slice]) == %{"b" => ["a", "b", "c"]}

Here slice is a valid subset of the columns because even though slice.last > n_cols, Enum.to_list(slice) == [1]. This only happens when step > 1 of course.

We can still try and work out a future proof approach though.

[josevalim]

I think you can adjust the last for a range start..finish//step, the last element can be calculated using this formula:
start + (div(finish - start, step) * step)

[billylanchantin]

it may be best to leave this to until we add a slice! to Elixir? 🤔

I think I'd prefer to merge this if possible. The semantics of out-of-bounds are currently different between Nx and Explorer. This PR attempts to bring them in line.

The other one is an empty range altogether. And then there are ranges that raise when out of index.

Sorry if I'm not understanding. To my eyes, 10..0//1 is both empty on its own and out of bounds for a 2-column dataframe and so should raise. Can you clarify what you think should happen with these two cases?

df = DF.new(a: [1], b: [2]) # 2 columns

df[1..0//1] # empty, but within bounds
df[9..0//1] # empty, but out of bounds

I think df[1..0//1] should return an empty dataframe and df[9..0//1] should raise. The other option is that all empty ranges (post normalizing) should return an empty dataframe regardless of bounds.

I admit this is a weird case and I don't feel strongly.

[josevalim]

I think that, if 1..10//10 should not raise for being outbounds because 10 is not actually included, so should not 10..1//1 for the same reason?

I agree that it doesn’t really matter which, but I think for those cases it should be consistent. I think always checking first and last, regardless of step, is the simplest way to go about it?

[billylanchantin]

I think that, if 1..10//10 should not raise for being outbounds because 10 is not actually included, so should not 10..1//1 for the same reason?

Ah great point. It's like whack-a-mole with these cases!

I think always checking first and last, regardless of step, is the simplest way to go about it?

I think you're right. I'll go ahead and do that.

The results will differ slightly from Enum.slice/2. But we need to do what makes sense for us.

[josevalim]

Probably best to think of slice as an implementation detail. I think my proposed implementation should work for checking indexes too!

[billylanchantin]

Probably best to think of slice as an implementation detail.

That makes sense!

I think my proposed implementation should work for checking indexes too!

It did! Hope it's ok that I played a little code golf with it. I realized yours was equivalent to:

if max(abs(first), abs(last)) >= n_cols do
  raise ...
end

[josevalim]

Super clean!