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.
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
Functional non-mutating methods. #46453
base: master
Are you sure you want to change the base?
Functional non-mutating methods. #46453
Changes from 8 commits
895dc59
7461359
94eb379
495ab02
44eba7e
c05019b
82af253
d7b0673
230e352
ee924a1
f7e9888
2e40121
94191a8
b5df3c7
3ca433a
57498dd
27304e0
abb003e
cd5a701
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
For vectors with statically known size (eg
SVector
s), the following alternative performs better and without allocations:Maybe, this should be used instead? And/or, add a similar method with
inds::NTuple{Integer}
?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 for the snippet! it now replaces the old commented code.
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.
Sorry, I must've gone crazy with that
fold
!This works the same for static arrays, better for regular arrays, and even seems efficient for tuples:
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.
and for unitranges
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.
Did we want to allow having tuples of indices be permitted as
inds
here?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.
Yeah maybe best without tuples.
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 that's correct.
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.
So,
findlast
instead offindfirst
? Or throw?Not clear which is the best semantics.
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.
?
Instead of all these methods.
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, I see - it may work with setindex-able, but not resizeable arrays as of now...
Unfortunate that so much duplication is needed.
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.
Yeah, it's a bit messy. I thought about putting
# TODO refactor when immutable arrays are in base
but I don't think we know how that's going to play out still. Also, I use "TODO" as a keyword in searches for my code bases, so I try not to pollute community projects with it unless it's clearly helpful for everyone.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 implementation is not quite correct since I can current;y construct an ImmutableDict where there are multiple entries for
key
each shadowed.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 suspect
delete
would be more easily done with adding a new key as a tombstone (with the samekey
, butval
being#undef
), rather than a bulk copy like this.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 really like that idea, but I think we would need to have a new type since we can't rely on
#undef
for bits types.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.
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.
There might be a little footgun hidden here, as
zip
does not check lengths:So I think there should be a length check.
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.
Good catch.
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.
Also allows duplicates, which getindex does not. Is this indended?
Only one test https://github.com/JuliaLang/julia/pull/46453/files#diff-a4ae685ea77cc80977dc4214e7dbb9d96fed55bdb918396d3453f840da0a695bR308
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 figured this was more like the behavior of
setindex!
where a value can be overwritten multiple times.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.
But get/set both allow repeats for arrays. I'm not sure whether
getindex
disallowing repeats here is a feature or a bug, either. Do you know?For arrays, the types must match:
setindex!(rand(10), (3, 4), 5:6)
is an error, rather than itererating the tuple, becausegetindex(rand(10), 5:6)
is a vector. Is there an argument for why this method should accept & iterate anything?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 it's just because you can't have repeated fields in a
NamedTuple
, so(x=1, y=2, z=3)[[:x, :y, :x]]
would have to make up a new symbol for the last:x
in order for it to work.I'm not completely sure what rules should carry over. You also can't index arrays with anything but integers and arrays of integers. I don't know if this means that anytime
setindex
sets multiple values the collection should be an array, or if the type should match the destination.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.
Ok I guess the lack of repeats in getindex is indeed forced on you by the return type.
The others I don't know, I just think they need careful thought.
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.
An alternative definition could be
which requires
vs
have a conversion toTuple
and matches the definition ofgetindex
more.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 differs from the error for tuples with out-of-bounds index. The logic is that
setindex
lets you add arbitrary keys? But not for integers?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 was following the behavior of
delete
which doesn't throw an error ifkey
isn't found.deleteat!
anddeleteat
do throw errors if the field/index isn't found.