Skip to content
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

add pop!(vector, idx, [default]) #35513

Merged
merged 2 commits into from
Apr 24, 2020
Merged

add pop!(vector, idx, [default]) #35513

merged 2 commits into from
Apr 24, 2020

Conversation

rfourquet
Copy link
Member

This removes and return an element at an arbitrary position of a vector, and seems to be compatible with the docstring for pop!(collection, key[, default]).

Python has it, and I needed it today, which seemed good enough arguments for me to add this :)

@rfourquet rfourquet added arrays [a, r, r, a, y, s] feature Indicates new feature / enhancement requests labels Apr 18, 2020
base/array.jl Outdated
Comment on lines 1134 to 1135
_deleteat!(a, i, 1);
x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

Suggested change
_deleteat!(a, i, 1);
x
_deleteat!(a, i, 1)
return x

?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I will make a compromise and remove the useless ; while not adding return ;-)
Adding return in long function form is not yet officially recommended and Base is full of cases without return, so I will contribute to support this lovely style here :)

base/array.jl Outdated
x
end

pop!(a::Vector, i::Integer, default) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boring comment but this looks like it should be a full length function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

@JeffBezanson JeffBezanson merged commit 745ad10 into master Apr 24, 2020
@JeffBezanson JeffBezanson deleted the rf/pop-array-index branch April 24, 2020 15:02
mbauman added a commit that referenced this pull request Apr 28, 2020
* origin/master: (833 commits)
  Improve typesubtract for tuples (#35600)
  Make searchsorted*/findnext/findprev return values of keytype (#32978)
  fix buggy rand(RandomDevice(), Bool) (#35590)
  remove `Ref` allocation on task switch (#35606)
  Revert "partr: fix multiqueue resorting stability" (#35589)
  exclude types with free variables from `type_morespecific` (#35555)
  fix small typo in NEWS.md (#35611)
  enable inline allocation of structs with pointers (#34126)
  SparseArrays: Speed up right-division by diagonal matrices (#35533)
  Allow hashing 1D OffsetArrays
  NEWS item for introspection macros (#35594)
  Special case empty covec-diagonal-vec product (#35557)
  [GCChecker] fix a few tests by looking through casts
  Use norm instead of abs in generic lu factorization (#34575)
  [GCChecker,NFC] run clang-format -style=llvm
  [GCChecker] fix tests and add Makefile
  Add introspection macros support for dot syntax (#35522)
  Specialize `union` for `OneTo` (#35577)
  add pop!(vector, idx, [default]) (#35513)
  bump Pkg version (#35584)
  ...
@rfourquet rfourquet added this to the 1.5 milestone May 3, 2020
@rfourquet
Copy link
Member Author

I'm having after-thougths on this one, because of the delete! vs deleteat! distinction, cf. #20531. Should this be called popat!?
As @nalimilan put it, "The generic definition of delete!() suits deleteat!() very well", similarly here the definition of pop! suits well the new method for vector, but the "subsequent items are shifted".

Also, I remembered that pop!(vector, idx) is equivalent to splice!(vector, idx), and pop!(vector, idx, default) seems less generally useful. I still find that pop!(vector, idx) looks nice to have (it's more discoverable), but not at the cost of being inconsistent...

@rfourquet rfourquet removed this from the 1.5 milestone May 3, 2020
@nalimilan
Copy link
Member

nalimilan commented May 4, 2020

It's an interesting case. ?pop! says "Remove an item in collection and return it. If collection is an ordered container, the last item is returned.", so it implies an order. Deleting the last element in an array doesn't affect the mapping of other keys/indices to values, contrary to other elements, so that's not different from calling pop!(dict, key). OTC, allowing to specify an index removes that guarantee, and would indeed justify using popat! instead, just like we have both delete! and deleteat!.

(I'm actually not completely convinced that distinction is essential, but that's our API at least until 1.x.)

@rfourquet
Copy link
Member Author

Note that there is a separate docstring for pop!(collection, key[, default]) though:

Delete and return the mapping for key if it exists in collection, otherwise return default, or throw an error if default is not specified.

It can easily be interpreted as working for vector, the unlear point being the fact that after deletion, there will be a new mapping for key because remaining elements are shifted.

@lesshaste
Copy link

lesshaste commented May 27, 2020

This is one of those features that arguably should not exist in Python. The performance is linear where many coders expect constant time for a pop operation.

@rfourquet
Copy link
Member Author

The performance is linear where many coders expect constant time for a pop operation.

I find this totally subjective. There are situtation when you need an O(n) operation, and this is such a case. I'm not against finding another name for this operation, e.g. like I suggested popat! (for another reason). But I expect a coder to know that different data structures have different complexity trade-offs, and I don't see why a coder would expect pop!(vector, idx) to be O(1) but expect deleateat!(vector, idx) to be O(n).

@lesshaste
Copy link

Am I right that it is O(1) if popping from the end?

It is subjective as you say. I was just reporting my experience teaching python coding.

@rfourquet
Copy link
Member Author

rfourquet commented May 27, 2020

Am I right that it is O(1) if popping from the end?

Yes fortunately :)

And indeed, I agree that it can be surprising to novice programmers, and I also know that you are not alone in thinking O(n) pop! is a bad idea (for example, I got push-back when I suggested a side-effect free push on vectors which would be O(n)).

@lesshaste
Copy link

That is what causes the confusion. Lots of python students at least just can't imagine how these things are implemented. To be fair to them, I don't know if it is O(1) time if popping the second last element without reading the source. It could be O(1) if removing the first element too but I assume not.

@nalimilan nalimilan added the triage This should be discussed on a triage call label May 27, 2020
@StefanKarpinski
Copy link
Member

From triage: we're not that concerned about the performance issue but we're pretty concerned about the semantic distinction between what pop!(dict, key) and pop!(vec, idx) do, which is analogous to the distinction between delete!(dict, key) and deleteat!(vec, idx)—namely that delete!(dict, key) has no affect on the other (key, val) mappings in dict whereas deleteat!(vec, idx) implicitly changes all (idx, val) pairs following the given idx value in vec to (idx-1, val). Of course, that semantic difference is precisely why there's O(n) performance, but if performance was the only difference we wouldn't care much about this.

@JeffBezanson
Copy link
Member

Given that this is on a release branch already, probably best to rename it to popat! instead of removing it completely?

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Jun 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] feature Indicates new feature / enhancement requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants