-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
rand of AbstractArray #8649
Closed
Closed
rand of AbstractArray #8649
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1d929d7
add cendof, cget, cset! functions
rfourquet d52ebff
re-add #8309 (which was reverted)
rfourquet b49f843
re-enable "full-range" rand
rfourquet b3b2b64
Merge branch 'master' into rand_range
rfourquet e6fcfeb
move cget/cendof to random.jl
rfourquet 4147213
rename endof/cget to endof_0/getindex_0
rfourquet 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
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
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
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.
You'll not get this use of
@inbounds
past Jeff. #8227 is still open about being able to propagate@inbounds
declarations trough functions in a meaningful way.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.
Thanks, but then I'm not sure what to do: rename this method to
cget_unsafe
instead? (in my mind, the "c" in "cget" was already a warning, and this is meant for internal use only). Or remove@inbounds
and hope that there is no/little overhead?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.
This is such a simple definition, I'm not sure what benefit there is to even having it if its strictly for internal use. Similarly,
cendof
just seems like a pointless private definition mixed in with a bunch of things that are public.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.
The functions
cendof
andcget
are needed only for their specializations for ranges (where the definition is slightly less trivial). In order for this PR's extendedrand
functionality to work with ranges likerand(typemin(Int):typemax(Int))
, there needs to be a way to index into every position of such a range (which is not particularly specific torand
). Implementingcget
/cendof
was the simplest solution I found, but I humbly request for help to improve the design of these functions or to find alternatives.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.
Stefan has a good point. In fact,
cget
is only used in two places, so it looks like it would be just as easy to specialize those cases for Ranges rather than introduce cget and cendof. At the very least I would move all the code forcget
to random.jl, since I certainly hope we won't see increasing use/need of 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.
It is better to start in the
Random
module in random.jl and move toBase
in abstractarray.jl if we need it in more places.range.jl
is also a possible place, as the problem is really that we wantUnitRange
to support a length oftypemax(Uint) + 1
, and other uses ofUnitRange
might also need to support full ranges.How about the names
length_0
andgetindex_0
instead?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 don't think
length_0
fits, as whatever the first indice is, the length remains the same, unlike the last indince; I could rename toendof_0
andgetindex_0
(I first thougth oflength_minus_1
or evenlength_1
, taking "_" as meaning "minus", but "end of" describes better what is needed).I can certainly move the code for
cget/cendof
in random.jl. I didn't do that at first as rand has nothing to do with these implementations (rand
needs it only not to break previously working functionality), and these could be used elsewhere also (e.g.sum(1:typemax(Uint))
works but notsum(0:typemax(Uint))
which should give the same answer, but I admit it's not a very convincing example). However I'd rather not haverand
have a specialization for ranges.