RFC: Split delete!(h::Dict,key) into pop!(), delete!() (#3405) #3439

Merged
merged 3 commits into from Aug 14, 2013

Projects

None yet

7 participants

@kmsquire
Member

As discussed in #3405, here are some exploratory changes to split delete!(h::Dict, key) into two functions.

  • pop!(h::Dict, key) returns the value removed from h, or throws an error if the key missing
  • pop!(h::Dict, key, default) returns the value removed from h, or default if the key is missing
  • delete!(h::Dict, key) discards the value and returns the modified dictionary; no error thrown on missing value.

Also implemented for Set(), IntSet(). Haven't touched ObjectIdDict or WeakKeyDict.

Edit: Added info about errors, default value on pop!()
Edit: Also updated ObjectIdDict, WeakKeyDict

@StefanKarpinski
Member

Nice, it seems like a good change and definitely pushes the conversation forward. Now it feels a little like push! should take an optional key, but that doesn't actually make sense for half of the collections. Perhaps we should break the general relationship between push! and pop! except for actual stacks. For example, for sets, push! could replace add! as a function to add an arbitrary item to the set, while pop! could be used to remove an arbitrary item from the set.

@kmsquire
Member

I think I've finished all of the needed pop! updates. I'd like to say all tests pass, but they're failing for other reasons (going to submit issues tonight or tomorrow).

At this point, I have some thoughts, and could use some feedback.

  1. Even though I proposed pop! (based on Python's usage), I'm not sold on push! as the opposite here.
  2. With this change, delete! returns the collection. While I like the change, it's pretty disruptive if someone is depending on the previous behavior. I added a deprecation for the three-argument version of delete!, and warnings for the 2-argument version. The warnings are quite annoying, so I think they should be removed in favor of loud warnings on the mailing list if/when this is merged
  3. Right now, delete! does not throw an error if the key is not in the collection--it just returns the unchanged collection. This might cause issues with previous code using the two-argument version of delete! and depending on an error being thrown if the key is not in the collection. I made one in-tree change from delete!(a,b) to pop!(a,b) because it was unclear whether or not that behavior was needed. It may be better to throw an error (though I prefer the current behavior--if someone needs an error thrown, they can use pop!).

Other ideas:

  1. rename delete! to remove!, to ease the transition
  2. use getrm! or getdel! instead of pop! (or some other name that does not imply the requirement of a push!--this is about splitting up delete! into two semantically different concepts, not implementing push! and pop! for all collections.)
  3. Or, just deal with pop! without push!. (Or implement push! separately!) FWIW, python has pop() but uses add() to add elements; there's no push() function.

Edit: push() was added (see below)

@JeffBezanson
Member

Shall we try to get this merged for 0.2?

@StefanKarpinski
Member

I feel that this generalizes the meaning of pop! in a way that leaves things in an inconsistent state. Should we just say that push! adds an element while pop! removes an element, and in the case of stack-like collections, there is a guarantee that x == pop!(push!(c,x)) and that c is left in the same state before and after? In that case Set add! can become a method of push! and pop! can just give you an arbitrary set element, removing it from the set. I personally think that's a pretty reasonable API but you seemed to have stronger feelings about pushing and popping satisfying specific invariants.

@kmsquire
Member

@StefanKarpinski, that sounds fine. I'll add the corresponding push! methods.

@kmsquire
Member

After staring at this a little, I'm wondering if there is need for a method to return both the modified collection and the removed element... For immutable collections, I think this would be required. Thoughts?

@StefanKarpinski
Member

Those versions wouldn't have the ! so are out of the scope of these particular functions.

@kmsquire
Member

True, that!

One more thought:

  • pop!(), as implemented here, returns the element, and modifies the collection
  • delete!() as implemented here, returns the modified collection
  • push!() currently returns the modified collection

This generally seems like reasonable behavior for push!(), but is the asymmetry with pop!() reasonable?

@StefanKarpinski
Member

I think the asymmetry of pop! and push! is traditional and sensible. This is the same sort of "balanced asymmetry" that read and write have.

@kmsquire
Member

Okay, makes sense.

In that case Set add! can become a method of push!

Can you clarify this statement? Is it that you want add! defined in terms of push!? push! to replace add!? Something else?

At this point, the decision is somewhat arbitrary, just wondering if you had something specific in mind.

@kmsquire
Member

Reading your earlier suggestion again, I'll deprecate add!.

@kmsquire
Member

Okay, I added push! for Sets and Dicts, deprecated add!, and removed jl_eqtable_del in response to @JeffBezanson's comment.

Tests pass (on my machine, at least).

The warnings are a bit annoying. The problem is that delete! didn't go away here, but its semantics changed, and the warnings highlight this. For Julia code, that shouldn't be an issue, but for user code, the warnings would be useful. It would be nice would be to turn on deprecations for user code only, but I'm not sure how feasible that is.

I can squash and/or turn off the warnings, although I think the change of meaning of delete! should be announced on the mailing list before this is merged. It would also be worthwhile to grep packages for potential conflicts.

@kmsquire
Member

This is also one of those changes where it would be nice if a package could depend on a particular non-release version of julia (cf #3465).

@toivoh
Member
toivoh commented Jul 11, 2013

Current add! semantics allow to add an already existing element to a set. Are we comfortable with letting push! do that, especially when it will always add a new element to a vector? I'm not sure what we are gaining by bringing add! and push! together.

@ViralBShah
Member

Worth keeping in mind that @lindahua is currently using add! on vectors for vector addition in NumericExtensions and we may bring that into Base.

@kmsquire
Member

Bump.

Given @JeffBezanson's recent comments, it seems like some version of this pull request will likely go into 0.2. Rebasing is slightly painful, so I'd rather not go through it too many more times. If someone can give a heads up when they want to review/try this more, I'll rebase again.

@JeffBezanson
Member

Let's merge this. @StefanKarpinski ?

@kmsquire
Member
kmsquire commented Aug 8, 2013

In the morning, I'll

  • fix the deprecation issue Jeff pointed out
  • remove the delete warning (its really annoying)
  • rebase

One other question: should delete!() throw an error if the key is not present? It currently just returns the unmodified dictionary.

@JeffBezanson
Member

I thought part of the purpose of delete! was to be silent if the key is not present.

@kmsquire
Member
kmsquire commented Aug 8, 2013

@JeffBezanson, you're absolutely right. I had somehow forgotten that that was one of the main points of this change.

@StefanKarpinski
Member

Give me a bit to look over this.

kmsquire added some commits Jun 18, 2013
@kmsquire kmsquire Split delete!(h::Dict,key) into pop!(), delete!()
* Add documentation for pop!(collection, key[, default])
* Deprecate Set add! in favor of push!, for symmetry with pop!
* Also: add push!(h::Dict, k, v) for symmetry with pop!(h::Dict)
78ec4d1
@kmsquire kmsquire Update Dict pop!/delete! changes to latest Julia, remove warnings 6171895
@kmsquire
Member
kmsquire commented Aug 8, 2013

I just rebased to the latest julia, and made a few minor changes. I didn't squash the last commit yet so you can see what these are. Tests pass for me (we'll see what Travis says).

As I pointed out before, because this changes the behavior of delete!, it would be good to announce it on the mailing list when it goes in. In the mean time, I'm installing all available packages and looking for uses of delete!.

@kmsquire
Member
kmsquire commented Aug 8, 2013

Re:Travis, clang passed, gcc failed. Passed with gcc on my machine.

cc: @staticfloat

@StefanKarpinski StefanKarpinski and 1 other commented on an outdated diff Aug 8, 2013
base/deprecated.jl
@@ -223,6 +222,9 @@ export PipeString
@deprecate finfer code_typed
@deprecate disassemble(f::Function,t::Tuple) code_llvm(f,t)
@deprecate disassemble(f::Function,t::Tuple,asm::Bool) (asm ? code_native(f,t) : code_llvm(f,t))
+@deprecate add(s::Set, x) push!
+@deprecate add!(s::Set, x) push!
@StefanKarpinski
StefanKarpinski Aug 8, 2013 Member

These deprecations are not quite right: the rhs needs to be push!(s,x).

@kmsquire
kmsquire Aug 8, 2013 Member

Whoops, right, thanks.

@staticfloat
Member

Behold; the Travis gods have deemed a re-run of the gcc job to be acceptable. If you have permissions to JuliaLang/julia.git, you can click on the little gear in the top right of a Travis job and click "re-run job" to do what I just did.

@kmsquire
Member
kmsquire commented Aug 8, 2013

@staticfloat, I saw the gear, but "re-run job" was grayed out.

@staticfloat
Member

You must not have permissions then. :/ I'm not sure how github permissions
map to Travis permissions even. Oh well. Hopefully we'll eventually track
down these bugs and won't need to do this anymore!
-E

On Thu, Aug 8, 2013 at 12:54 PM, Kevin Squire notifications@github.comwrote:

@staticfloat https://github.com/staticfloat, I saw the gear, but
"re-run job" was grayed out.


Reply to this email directly or view it on GitHubhttps://github.com/JuliaLang/julia/pull/3439#issuecomment-22350691
.

@JeffBezanson
Member

bump

@kmsquire
Member
kmsquire commented Aug 9, 2013

I'm not quite finished installing all packages (Pkg2 is still slow, and until a moment ago, I wasn't smart enough to catch errors for uninstallable package, so I kept on restarting it).

A quick grep shows that the number of packages using delete! in a way that will break is small, but one of them is BinDeps (cc: @loladiro). I'll post a complete list in a little while.

@kmsquire
Member
kmsquire commented Aug 9, 2013

The packages below all use delete!. The maintainers of these packages should read through this pull request and be aware of the changes they may need to make.

Packages using the return value of delete!
BinDeps (cc: @loladiro)
HDFS (cc: @tanmaykm)
HTTP (cc: @dirk)

Packages which may want to make the same change:
Images (cc: @timholy)
DataFrames (cc: @johnmyleswhite, @harlanh)
JudyDicts (cc: @tanmaykm)
PyCall (cc: @stevengj)
SimJulia (cc: @BenLauwens)

All packages using delete! (most uses okay)
BinDeps (@loladiro)
Blocks (@tanmaykm)
ChainedVectors (@tanmaykm)
DataFrames (@JuliaStats)
Debug (@toivoh)
DictUtils (@slycoder)
FunctionalCollections (@zachallaun)
GLPK (@carlobaldassi)
Gtk (@JuliaLang)
HDFS (@tanmaykm)
HTTP (@dirk)
HttpServer (@hackerschool)
IJulia (@JuliaLang)
Images (@timholy)
JudyDicts (@tanmaykm)
Mustache (@jverzani)
NetCDF (@meggart)
PyCall (@stevengj)
PySide (@jverzani)
RandomMatrices (@jiahao)
SimJulia (@BenLauwens)
TextAnalysis (@johnmyleswhite)
YAML (@dcjones)


Uses of delete! which

  1. ignore the return value and
  2. do not care if an error is not thrown
    do not need to change anything

Before this patch (#3439) is applied, uses of delete! which use the return value, such as

x = delete!(dict, value[, default])

or want a KeyError() thrown may want to change these uses to something like the following so that their code does not break when this patch is applied:

x = get(dict, value, default)
if x == default
   throw KeyError("$value not found in dictionary")
end
delete!(dict, value)

After this patch is applied, this can simply be changed to

x = pop!(dict, value[, default])
@kmsquire
Member
kmsquire commented Aug 9, 2013

These should have been copied in the last note...

CC: @toivoh!, @slycoder!, @zachallaun!, @carlobaldassi!, @hackerschool!, @jverzani!, @meggart!, @jiahao!, @dcjones

@stevengj
Member
stevengj commented Aug 9, 2013

Should that be get and not get! in your suggested workaround?

@kmsquire
Member

It should, thanks. Fixed in the issue.

@stevengj
Member

Wouldn't it be cleaner to do:

if method_exists(pop!, (Dict, Any, Any)) # from Julia #3439
    mypop!(d::Dict, x, y) = pop!(d, x, y)
    mypop!(d::Dict, x) = pop!(d, x)
else
    mypop!(d::Dict, x, y) = delete!(d, x, y)
    mypop!(d::Dict, x) = delete!(d, x)
end

and then just use mypop! until it is safe to just change to pop!?

For my part, I'm happy just waiting for this patch to land and then updating PyCall within the next day or so. Until Julia 0.2 is released, backwards compatibility is not worth messy code for me.

@kmsquire
Member

That would also be fine. And since you're in charge of your package, doing nothing and letting things break for a little while is fine as well.

I tend to want to coddleaccommodate users who are using a julia 0.2 pre-release for real work, without updating the main code base, but who occasionally update modules, and find that they break for reasons like this. I don't think it puts julia in a good light.

But I keep getting pushback on this, and I admit that 1) it's rather annoying and 2) is mostly just creating extra work for maintainers at this early stage of julia's evolution. So now that most relevant parties have been notified, I guess we should just merge this and pick up the pieces and be done with it. (After @StefanKarpinski gives his blessing.)

(There also aren't as many uses as I first feared.)

@kmsquire
Member

@StefanKarpinski, do you plan to review this any more? I'd like to make a general announcement on the mailing list, and pull the trigger a day or two later.

@StefanKarpinski
Member

Yes, sorry for the slowness.

@kmsquire
Member

No worries, I'll wait.

@StefanKarpinski
Member

Ok, just went over this with @JeffBezanson and I think we should follow through on this. It leads, however, to further logical changes, such as replacing shift!(c) with pop!(c,1) and unshift!(c,x) with push!(c,1), but those can be made after this is merged. This is a bit of a complex merge, @kmsquire, so go ahead whenever you want.

@StefanKarpinski
Member

Ah, hell. I'm just merging.

@StefanKarpinski StefanKarpinski merged commit e421dc3 into JuliaLang:master Aug 14, 2013

1 check passed

default The Travis CI build passed
Details
@kmsquire
Member

@StefanKarpinski, thanks for merging. I was hoping to submit a pull request for at least BinDeps, which will break because of this change, but I guess that can happen now. (cc: @loladiro)

@kmsquire kmsquire deleted the kmsquire:dict_pop branch Aug 14, 2013
@stevengj
Member

PS. Why is it pop!(d, k) to remove d[k], but splice!(a, i) to remove a[i]?

@stevengj stevengj added a commit to JuliaPy/PyCall.jl that referenced this pull request Aug 14, 2013
@stevengj stevengj update delete/pop semantics for JuliaLang/julia#3439 cf3fe58
@JeffBezanson
Member

I think pop!(a, i) on arrays could also be defined; its functionality is a subset of splice!. The possible objection is that on arrays popping a non-final element shifts other values, while in sets and dicts only the removed value is affected.

@stevengj stevengj added a commit that referenced this pull request Aug 14, 2013
@stevengj stevengj update NEWS for #3439 68df9ee
@MichaelHatherly MichaelHatherly pushed a commit to JuliaPackageMirrors/HDFS.jl that referenced this pull request Oct 2, 2015
@kmsquire kmsquire Update hdfs_jobs.jl
Update delete! usage in response to 
JuliaLang/julia#3439
7eb8f9f
@MichaelHatherly MichaelHatherly pushed a commit to JuliaPackageMirrors/HTTP.jl that referenced this pull request Oct 2, 2015
@kmsquire kmsquire Update BasicServer.jl
Update delete! usage in response to JuliaLang/julia#3439
e17f545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment