-
Notifications
You must be signed in to change notification settings - Fork 126
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 on out of bounds range access #1061
Open
billylanchantin
wants to merge
8
commits into
main
Choose a base branch
from
bl-raise-on-oob-range
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+41
−7
Open
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
fe34ab4
Change behavior to raise on out of bounds
billylanchantin 2645aeb
Add test of new error message
billylanchantin 883db58
Account for slice semantics a different way
billylanchantin 5ab6085
Fix typos
billylanchantin ad35253
Also update tests to fix typos there
billylanchantin 0be99a5
Account for "pseudo max" on ranges
billylanchantin dc059a1
Account for reverse ranges in `Enum.slice/2`
billylanchantin b28645b
Both indices need to be in bounds
billylanchantin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. If the code above works (big if), then it should be simpler and more future proof. Worth giving it a try?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! Unfortunately though, I think your algorithm doesn't work for the following situation:
Here
slice
is a valid subset of the columns because even thoughslice.last > n_cols
,Enum.to_list(slice) == [1]
. This only happens whenstep > 1
of course.We can still try and work out a future proof approach though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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?I think
df[1..0//1]
should return an empty dataframe anddf[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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah great point. It's like whack-a-mole with these cases!
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably best to think of slice as an implementation detail. I think my proposed implementation should work for checking indexes too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense!
It did! Hope it's ok that I played a little code golf with it. I realized yours was equivalent to: