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

Rename shift! and unshift! ? #23902

Closed
c42f opened this issue Sep 27, 2017 · 59 comments
Closed

Rename shift! and unshift! ? #23902

c42f opened this issue Sep 27, 2017 · 59 comments

Comments

@c42f
Copy link
Member

c42f commented Sep 27, 2017

As mentioned on slack a few days ago, unshift! may be inherited, but it's a reasonably poor name IMHO. As pre-1.0 is the time for fixing these things, how about it?

Since prepend is taken, how about unshift!->pushfront! and shift!->popfront!?

@StefanKarpinski
Copy link
Member

We could do a wholesale rename of these functions:

old new
push! rpush!
pop! rpop!
unshift! lpush!
shift! lpop!

@bramtayl
Copy link
Contributor

bramtayl commented Sep 27, 2017

Or a left/right (start/end? first/last?) keyword if performance issues could be worked out

@Sacha0
Copy link
Member

Sacha0 commented Sep 27, 2017

Left and right seem a bit ambiguous; perhaps @c42f's front/back or something similar would be clearer?

@StefanKarpinski
Copy link
Member

I'm not sure that front and back are clearer than left and right. Of course, for left/right terminology it's a bit unfortunate that we print vectors vertically, but front/back aren't better in that respect.

@StefanKarpinski
Copy link
Member

Oh, and we already use l and r for left and right in reductions, so there's precedent – unless we want to change those too.

@Sacha0
Copy link
Member

Sacha0 commented Sep 27, 2017

I'm not sure that front and back are clearer than left and right.

I'm not sure either. front associates with lower indices (like Base.front) and back with higher indices in my mind, but I am sure that sort of association varies.

Perhaps better ideas: head-tail and first-last seem distinctly less ambiguous given their preexisting meanings?

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Sep 27, 2017

The names hpush!, hpop!, tpush! and tpop! aren't bad. However we already associate left and right with the ordering of arrays in foldl and foldr, etc. Actually, since those have the l and r at the end, the analogous names are pushl!, popl!, pushr! and popr! which I kind of enjoy :)

@c42f
Copy link
Member Author

c42f commented Sep 28, 2017

Out of the options so far for renaming all four functions, the suffixed versions pushl!, popl!, pushr! and popr! look best to me.

I have some reservations about renaming push! and pop! though. For example, consider pushing and popping to a priority queue. In this case there's no "right" and "left", there's only a natural ordering and a data structure which returns the least/greatest element. There's also the version of pop! which takes a key for use with dictionary like data structures, and popr! would also not make much sense as a verb in that case.

@quinnj
Copy link
Member

quinnj commented Sep 28, 2017

I think push! and pop! can stay and be used by PriorityQueues. They can also just alias pushr! and popr! for Arrays.

@moldray
Copy link

moldray commented Sep 28, 2017

I love shift! & unshift! , it is the same with javascript. please reserve this!

@KristofferC
Copy link
Member

So far, I like the first post's suggestion the best.

@stevengj
Copy link
Member

I think it's good to use push*! and pop*! names to emphasize the similarity between the variants, and so that the variants can be found easily by tab completion. (I like the r and l suffix suggestion.)

@JeffBezanson
Copy link
Member

-2 to r and l terminology. The names are a bit ugly, and there are 4 of them. I would rather only rename shift and unshift like the OP suggests. But I also feel shift and unshift are used widely enough that we might as well just keep them. It would be unfortunate to introduce terms nobody else uses for such incredibly common operations.

@c42f
Copy link
Member Author

c42f commented Sep 28, 2017

@JeffBezanson C++ vector uses push_front, so there is some precedent for this.

I assume the existing names can be traced back to the shift builtin in sh, and presumably perl picked this up and invented unshift (http://www.perlmonks.org/?node_id=613144) Other languages (php, javascript, ...?) followed so there's plenty of precedent.

@cormullion
Copy link
Contributor

I prefer proper English words... :) (unshift is in the OED dating back to 1972.)

@Gnimuc
Copy link
Contributor

Gnimuc commented Sep 30, 2017

or at least mention shift! & unshift! in pop! & push!'s doc:

help?> pop!
search: pop! popdisplay apropos deepcopy precompile __precompile__ peakflops promote_type
also see: `shift!` `unshift!` `push!`

  pop!(collection, key[, default])

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

@TotalVerb
Copy link
Contributor

Another idea: push/pop!(first, x, ...) and push/pop!(last, x, ...).

Theoretically, this syntax generalizes well to things like pop!(min, x).

@garrison
Copy link
Member

or at least mention shift! & unshift! in pop! & push!'s doc:

Related to #23789

@StefanKarpinski
Copy link
Member

Cool idea, @TotalVerb!

@nalimilan
Copy link
Member

Or merge with splice!/insert!, with first and last as pseudo-indices?

@c42f
Copy link
Member Author

c42f commented Oct 1, 2017

Another idea: push/pop!(first, x, ...)

Ah what? Ok this is a really creative idea :-)

It does seem pretty odd to use the functions first and last purely as syntax, rather than having anything to do with the actual code inside them. It's kind of great, but I'm not sure this kind of thing should encouraged in the ecosystem as it can lead to weird entanglements? For example, if you ever wanted to rename functions which were being used as syntax, what then? Admittedly not at all likely for first and last, but still.

@StefanKarpinski
Copy link
Member

I have to say, as cool as I think this idea is – it reminds me of @mbauman's idea to use reduction functions in indexing, as in X[mean, :, sum] – I think it's too fancy for such a fundamental operation. I guess the options I'm in favor of at this point are:

  1. Leave things as they are – iffy as "shift" and "unshift" are, they're quite standard.
  2. Rename shift! and unshift! to popl! and pushl!.
  3. In addition to 2, rename pop! and push! to popr! and pushr!.
  4. In addition to 3, make pop! and push! aliases for popr! and pushr!.

I'm largely between 1 and 2 but there's some appeal to 3 and 4 as well.

@mkborregaard
Copy link
Contributor

I think that 1) it makes a lot of sense that applying the same operation (pushing or popping) to the beginning and end of an array should have at least related names; push! and shift! are an unintuitive pair; 2) the word shift! can be confounded with bit shifting >>, as can easily be seen by searching the docs for "shift"; 3) many Julia users won't have a perl/javascript background anyway.
I'm thus in favour of option 2 in the post above.
But, if we rename them, what to do about circshift!?

@stevengj
Copy link
Member

stevengj commented Dec 11, 2017

Stefan's option 2 or 4 seem good to me. I don't think circshift! needs to change, because it's not a pop-like operation

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 11, 2017

Proposal:

old new aliases
push! pushr! push!
pop! popr! pop!
unshift! pushl!
shift! popl!

I have to say that's tempting, especially if we keep push! and pop! as aliases for the cases where you're only using a structure as a stack and don't really care whether it's the left or right you're pushing to and popping from.

@KristofferC
Copy link
Member

Imo using front and back are (significantly) clearer than abbrevating left and right.

@vtjnash
Copy link
Member

vtjnash commented Dec 11, 2017

Especially since our vectors are vertical (so it should be top/bottom) 🙂. Other options might be start/end or first/last.

@stevengj
Copy link
Member

left/right seems fine to me; it's consistent with our reduction terminology (foldl and foldr), and with how vectors are typically entered [a,b,c,…] and output by print.

The other nice thing about left/right is that it has a well-established one-character abbreviation.

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Dec 11, 2017

Seriously, I don't know which end of a vector is the front or the back. Is the front index 1 or is that the back? Yes, we sometimes print our vectors vertically, but we also print them left-to-right in other cases. We're already consistently using inherited left/right naming for many other functions.

@phaverty
Copy link
Contributor

How about just having new methods for append! and prepend! where the second argument is a scalar?

@pabloferz
Copy link
Contributor

pabloferz commented Dec 11, 2017

I like best the popfirst!, pushfirst!, poplast!, pushlast! with pop!and push! as aliases for the two latter.

@StefanKarpinski
Copy link
Member

where the second argument is a scalar?

It's not possible to define scalar generically.

@phaverty
Copy link
Contributor

"It's not possible to define scalar generically."

Ah, I see. You could specify, say,

append!( x::Vector{T}, y::T )

, but you really want y to be "something that can be converted to T". But, the type system doesn't (yet?) know what set of types can be converted. Bummer.

@StefanKarpinski
Copy link
Member

Let's have a (nonbinding, informative) vote:

  • 👍 for pushr!, popr!, pushl! and popl! with push! and pop! as aliases for the first two
  • 👎 for keep things as they are
  • 😕 for some other option

@Sacha0
Copy link
Member

Sacha0 commented Dec 12, 2017

That choice of emoji is somewhat biasing ;).

@KristofferC
Copy link
Member

And 👍 on OPs suggestion if you like that best ;)

@omus
Copy link
Member

omus commented Dec 13, 2017

I have a few thoughts on renaming these functions so I'll just apologize up front for possibly adding some noise into this conversation. So far Julia's names of push!, pop!, shift!, and unshift! functions are the same as Perl, Ruby, and JavaScript (https://en.wikipedia.org/wiki/Double-ended_queue#Operations). Personally I don't mind these names as I originally learnt them in Perl but I fully admit that shift/unshift are not memorable. There also appears to be a camp which thinks push/pop is a bad complementary pair.

@omus
Copy link
Member

omus commented Dec 13, 2017

Another idea: push/pop!(first, x, ...) and push/pop!(last, x, ...).

If we were to go in this direction I would suggest we use enqueue! and dequeue! as the names of these functions.

@omus
Copy link
Member

omus commented Dec 13, 2017

What I like about the original names is that they just use one word. Trying to keep to one word but make some nicer pairings I came up with push! and pull! for FIFO and push! and pop! for FILO. This would make push/pop work on the beginning of a queue instead of the end:

Old New
push! put!
pop! pull!
shift! push!
unshift! pop!

We could leave push/pop on the end and move put/pull to the beginning to be less breaking but I think that would leave things somewhat confusing.

@omus
Copy link
Member

omus commented Dec 13, 2017

Another alternative is to use append! and prepend! to respectively refer to push! and shift!. I don't have any reasonable words for dequeuing however and behead and curtail were the best I could come up with...

@JeffBezanson
Copy link
Member

@phaverty As long as it's possible to have a Vector{Any}, that will be ambiguous.

I'll repost this: #23902 (comment)

@omus Renaming shift! to push! seems gratuitously confusing.

append! operates on two collections, while push! et. al. operates on one element, so IMO that name just can't be used here.

@omus
Copy link
Member

omus commented Dec 13, 2017

Finally, we could always embrace unicode and go with something like:

ASCII Unicode Unicode name Alias
push! leftwards arrow to bar
pop! rightwards arrow from bar \mapsto
shift! rightwards arrow to bar
unshift! rightwards arrow from bar \mapsfrom

Think of the bar in the unicode as the queue. Unfortunately these unicode characters don't have nice aliases in the REPL from what I can tell.

@omus
Copy link
Member

omus commented Dec 13, 2017

Renaming shift! to push! seems gratuitously confusing.

Honestly, I would be unhappy if we actually took that approach. That approach definitely is trying to be nice to newcomers and really mean to existing users of Julia.

@stevengj
Copy link
Member

stevengj commented Dec 13, 2017

@omus, e.g. ↦ is \mapsto.

@omus
Copy link
Member

omus commented Dec 13, 2017

Ok, I lied: I have one more thing to say. As a more serious suggestion is we could go with this approach. It loses the nice push/pop but is far clearer as to the operation going on:

Old New
push! lastin!
pop! lastout!
shift! firstin!
unshift! firstout!

@omus
Copy link
Member

omus commented Dec 13, 2017

Feel free to down vote most of these suggestions. After doing some digging on this topic the only true conclusion I came to is that computer scientists embraced push/pop terminology and had no idea what to name shift/unshift.

@bramtayl
Copy link
Contributor

Now that we have constant folding, keyword arguments for this should be fast, correct? I don't understand why we would have 4 functions for this instead of 2, especially since multi-word functions are specifically discouraged to promote refactoring.

@StefanKarpinski
Copy link
Member

Entertaining as this is, it seems that we cannot do better than the status quo here.

@Sacha0
Copy link
Member

Sacha0 commented Dec 13, 2017

Entertaining as this is, it seems that we cannot do better than the status quo here.

The amount of support for the OP seems substantial? Perhaps closing this issue is a bit premature? :)

@KristofferC
Copy link
Member

Yes, the OP's suggestion has 11 up and 0 down...

@StefanKarpinski
Copy link
Member

I interpreted those as support for the general notion of renaming, not for that specific suggestion.

@c42f
Copy link
Member Author

c42f commented Dec 13, 2017

It's clear to me that after all this discussion we didn't find a really compelling alternative.

FWIW pushfront! is still my favorite and a worthwhile renaming in my opinion, but that might just be my C++ bias.

@timholy
Copy link
Member

timholy commented Dec 13, 2017

I like pushfront! and popfront!.

@c42f
Copy link
Member Author

c42f commented Dec 15, 2017

Well, I made a PR in #25100 to use pushfront!/popfront! as I think it's the minimal thing we can do to rename unshift!. Really the only thing unshift! has going for it is (a) it's the status quo, and (b) it has precedent in perl/javascript/php/other languages inheriting dubious choices from perl

Feel free to shoot it down ;-)

@omus
Copy link
Member

omus commented Dec 15, 2017

Similar to what @Sacha0 mentioned in #25100. I think if we really want to make a rename I suggest going with:

enqueue!(::typeof(first), a, item) = unshift!(a, item)
enqueue!(::typeof(last), a, item) = push!(a, item)
dequeue!(::typeof(first), a) = shift!(a)
dequeue!(::typeof(last), a) = pop!(a)
julia> enqueue!(::typeof(first), a, item) = unshift!(a, item);

julia> enqueue!(::typeof(last), a, item) = push!(a, item);

julia> dequeue!(::typeof(first), a) = shift!(a);

julia> dequeue!(::typeof(last), a) = pop!(a);

julia> foo = [1, 2, 3]
3-element Array{Int64,1}:
 1
 2
 3

julia> enqueue!(first, foo, 0)
4-element Array{Int64,1}:
 0
 1
 2
 3

julia> dequeue!(first, foo)
0

julia> enqueue!(last, foo, 5)
4-element Array{Int64,1}:
 1
 2
 3
 5

julia> dequeue!(last, foo)
5

@omus
Copy link
Member

omus commented Dec 15, 2017

Alternatively:

push!(::typeof(first), a, item) = unshift!(a, item)
push!(::typeof(last), a, item) = push!(a, item)
pop!(::typeof(first), a) = shift!(a)
pop!(::typeof(last), a) = pop!(a)

We could also keep push! and pop! around without passing in first or last which would default to push!(last, ...) and pop!(last, ...).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests