consider consistently using ! for mutating functions #907

Closed
StefanKarpinski opened this Issue May 31, 2012 · 20 comments

Comments

Projects
None yet
6 participants
@StefanKarpinski
Member

StefanKarpinski commented May 31, 2012

E.g. rename push => push!, etc. Discussion here.

@diegozea

This comment has been minimized.

Show comment
Hide comment
@diegozea

diegozea Jan 6, 2013

Member

I think the same, actually... I was thinking in open an issue like this!
I'm extending the dequeue function for my types, and the actual definitions looks awkward...
I'm think that push, del and others must be to be push!, del!, etc.
And returning the modified input in a function that modified the input looks awkward (redundant) and is inefficient (without not reason).

I'm opened this topic on julia-dev:
[https://groups.google.com/forum/?hl=es&fromgroups=#!topic/julia-dev/wKIlMaSj0YE]

Member

diegozea commented Jan 6, 2013

I think the same, actually... I was thinking in open an issue like this!
I'm extending the dequeue function for my types, and the actual definitions looks awkward...
I'm think that push, del and others must be to be push!, del!, etc.
And returning the modified input in a function that modified the input looks awkward (redundant) and is inefficient (without not reason).

I'm opened this topic on julia-dev:
[https://groups.google.com/forum/?hl=es&fromgroups=#!topic/julia-dev/wKIlMaSj0YE]

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Jan 6, 2013

Member

"returning the modified input...is inefficient": Prove it.

Member

JeffBezanson commented Jan 6, 2013

"returning the modified input...is inefficient": Prove it.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jan 6, 2013

Member

I'm not sure where the idea that returning the array is inefficient comes from, but that's just not the case.

Member

StefanKarpinski commented Jan 6, 2013

I'm not sure where the idea that returning the array is inefficient comes from, but that's just not the case.

@diegozea

This comment has been minimized.

Show comment
Hide comment
@diegozea

diegozea Jan 6, 2013

Member

I do a very simple test for push against push! in the mail list [ previous link ] yesterday at 3 AM. But, yes... Excuse me... Looks to be my error [ it's lees than a 1% better, probably only measurement error ]. Here a test for:

function enqueue!{T}(a::Array{T,1}, item)
    if is(T,None)
        error("[] cannot grow. Instead, initialize the array with \"T[]\".")
    end
    item = convert(T, item)
    ccall(:jl_array_grow_beg, Void, (Any, Uint), a, 1)
    a[1] = item
    nothing
end

And the results are:

        BenchmarkCategory BenchmarkName Iterations Fun_wins_to_Fun2 Fun2_wins_to_Fun    Ties  TotalWall AverageWall    MaxWall MinWall             Timestamp            JuliaVersion    JuliaHash CodeHash      OS
[1,]     "compare_return"        "with"      50220          4.60024          4.78505 90.6147 0.00282693  5.62909e-8   9.799e-5     0.0 "2013-01-06 19:51:44" "0.0.0+106484954.r5387" "53872cd2c9"       NA "Linux"
[2,]     "compare_return"     "without"      49780          4.60024          4.78505 90.6147 0.00268102  5.38573e-8 5.38826e-5     0.0 "2013-01-06 19:51:44" "0.0.0+106484954.r5387" "53872cd2c9"       NA "Linux"

But, I still think that the names need to be ending in ! for indicate modification of input data.

Member

diegozea commented Jan 6, 2013

I do a very simple test for push against push! in the mail list [ previous link ] yesterday at 3 AM. But, yes... Excuse me... Looks to be my error [ it's lees than a 1% better, probably only measurement error ]. Here a test for:

function enqueue!{T}(a::Array{T,1}, item)
    if is(T,None)
        error("[] cannot grow. Instead, initialize the array with \"T[]\".")
    end
    item = convert(T, item)
    ccall(:jl_array_grow_beg, Void, (Any, Uint), a, 1)
    a[1] = item
    nothing
end

And the results are:

        BenchmarkCategory BenchmarkName Iterations Fun_wins_to_Fun2 Fun2_wins_to_Fun    Ties  TotalWall AverageWall    MaxWall MinWall             Timestamp            JuliaVersion    JuliaHash CodeHash      OS
[1,]     "compare_return"        "with"      50220          4.60024          4.78505 90.6147 0.00282693  5.62909e-8   9.799e-5     0.0 "2013-01-06 19:51:44" "0.0.0+106484954.r5387" "53872cd2c9"       NA "Linux"
[2,]     "compare_return"     "without"      49780          4.60024          4.78505 90.6147 0.00268102  5.38573e-8 5.38826e-5     0.0 "2013-01-06 19:51:44" "0.0.0+106484954.r5387" "53872cd2c9"       NA "Linux"

But, I still think that the names need to be ending in ! for indicate modification of input data.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jan 8, 2013

Member

I'm proposing the following renames:

push    => push!
pop     => pop!
grow    => grow!
enqueue => unshift!
unshift => unshift!
insert  => insert!
del     => delete!
del_all => deleteall! (or clear!?)

As a transitional step, the old versions will be kept around with warnings.

Member

StefanKarpinski commented Jan 8, 2013

I'm proposing the following renames:

push    => push!
pop     => pop!
grow    => grow!
enqueue => unshift!
unshift => unshift!
insert  => insert!
del     => delete!
del_all => deleteall! (or clear!?)

As a transitional step, the old versions will be kept around with warnings.

@diegozea

This comment has been minimized.

Show comment
Hide comment
@diegozea

diegozea Jan 8, 2013

Member

Good!! 👍

I think deleteall! is better than clear! [ clear! gives the idea of deletion of data but maintain the structure (type and size) ]

Member

diegozea commented Jan 8, 2013

Good!! 👍

I think deleteall! is better than clear! [ clear! gives the idea of deletion of data but maintain the structure (type and size) ]

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Jan 8, 2013

Member

Here are my notes from our discussion about !, especially how it relates to linear algebra.

! convention:
after f!(A), A should equal B in B=f(A)

constructor convention:
Type(A) takes ownership of A and can mutate it.

linear algebra functions that might throw errors should be designed as follows:
a low-level function/ctor CholeskyDense(A) that takes A, mutates it, and
returns something that can tell you whether A was posdef. Does NOT throw.

a high-level function chol(A) and/or chol!(A) that throws an error if there
is a problem. it might have to do CholeskyDense(copy(A)) internally.

Member

JeffBezanson commented Jan 8, 2013

Here are my notes from our discussion about !, especially how it relates to linear algebra.

! convention:
after f!(A), A should equal B in B=f(A)

constructor convention:
Type(A) takes ownership of A and can mutate it.

linear algebra functions that might throw errors should be designed as follows:
a low-level function/ctor CholeskyDense(A) that takes A, mutates it, and
returns something that can tell you whether A was posdef. Does NOT throw.

a high-level function chol(A) and/or chol!(A) that throws an error if there
is a problem. it might have to do CholeskyDense(copy(A)) internally.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Jan 8, 2013

Member

How can CholeskyDense(A) indicate whether it worked or not? It's supposed to return an object of type CholeskyDense so it seems like there would have to be a notion of a CholeskyDense object that somehow encodes the fact that A wasn't positive definite?

Member

StefanKarpinski commented Jan 8, 2013

How can CholeskyDense(A) indicate whether it worked or not? It's supposed to return an object of type CholeskyDense so it seems like there would have to be a notion of a CholeskyDense object that somehow encodes the fact that A wasn't positive definite?

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Jan 8, 2013

Member

I was thinking CholeskyDense could have a field that tells you, but the important thing is some low-level function that doesn't throw an error so you have the option of more control.

Member

JeffBezanson commented Jan 8, 2013

I was thinking CholeskyDense could have a field that tells you, but the important thing is some low-level function that doesn't throw an error so you have the option of more control.

@andreasnoack

This comment has been minimized.

Show comment
Hide comment
@andreasnoack

andreasnoack Jan 8, 2013

Member

@JeffBezanson wrote:
! convention:
after f!(A), A should equal B in B=f(A)

This is not the case in lapack.jl and I think it could be too restrictive. The LAPACK functions return tuples typically with a matrix and error information and for example syevr! just destroys A and returns eigenvalues. Otherwise many of the LAPACK functions should copy their input and have ! removed.

@StefanKarpinski CholeskyDense gives and error if the decomposition fails but LUDense and CholeskyDensePivoted don't. They just save the information variable returned from LAPACK. For the former it is used in the det method and the latter has a rank method but fails if the factorisation is used for solving a system of equations.

Member

andreasnoack commented Jan 8, 2013

@JeffBezanson wrote:
! convention:
after f!(A), A should equal B in B=f(A)

This is not the case in lapack.jl and I think it could be too restrictive. The LAPACK functions return tuples typically with a matrix and error information and for example syevr! just destroys A and returns eigenvalues. Otherwise many of the LAPACK functions should copy their input and have ! removed.

@StefanKarpinski CholeskyDense gives and error if the decomposition fails but LUDense and CholeskyDensePivoted don't. They just save the information variable returned from LAPACK. For the former it is used in the det method and the latter has a rank method but fails if the factorisation is used for solving a system of equations.

StefanKarpinski added a commit that referenced this issue Jan 9, 2013

bang: consistently use ! for mutating functions [#907].
This deprecates the non-! versions and prints a warning when they
are used. It doesn't quite close out #907 but comes close. There
are still a few lingering mutators like del_each that we may want
to get rid of or rename somehow.

JeffBezanson added a commit that referenced this issue Feb 6, 2013

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Feb 6, 2013

Member

All done now, except for assign! which I did on a branch since it is very disruptive.

Member

JeffBezanson commented Feb 6, 2013

All done now, except for assign! which I did on a branch since it is very disruptive.

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Mar 9, 2013

Member

How are we feeling about assign!?

Member

JeffBezanson commented Mar 9, 2013

How are we feeling about assign!?

@johnmyleswhite

This comment has been minimized.

Show comment
Hide comment
@johnmyleswhite

johnmyleswhite Mar 9, 2013

Member

I'm for it.

Member

johnmyleswhite commented Mar 9, 2013

I'm for it.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Mar 9, 2013

Member

I think we should consider both ref and assign and make them match in some sense. Something like getindex and setindex!. Or maybe atindex and atindex!. Or perhaps we should go Pythonic on this with __index__ and __index__!. Generally, I'd say ick, but this is one of those cases where we really want to avoid collisions in a global sense.

Member

StefanKarpinski commented Mar 9, 2013

I think we should consider both ref and assign and make them match in some sense. Something like getindex and setindex!. Or maybe atindex and atindex!. Or perhaps we should go Pythonic on this with __index__ and __index__!. Generally, I'd say ick, but this is one of those cases where we really want to avoid collisions in a global sense.

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Mar 9, 2013

Member

I like getindex and setindex! best so far. Remember we want to leave the door open for a non-mutating setindex; there are nifty implementations of vectors that allow that.

Member

JeffBezanson commented Mar 9, 2013

I like getindex and setindex! best so far. Remember we want to leave the door open for a non-mutating setindex; there are nifty implementations of vectors that allow that.

@StefanKarpinski

This comment has been minimized.

Show comment
Hide comment
@StefanKarpinski

StefanKarpinski Mar 9, 2013

Member

Oh, fair enough. Yeah, let's go with getindex and setindex! then, leaving setindex as the non-mutating variant on setindex!.

Member

StefanKarpinski commented Mar 9, 2013

Oh, fair enough. Yeah, let's go with getindex and setindex! then, leaving setindex as the non-mutating variant on setindex!.

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Mar 9, 2013

Member

This is going to be the most vicious renaming ever.

Member

JeffBezanson commented Mar 9, 2013

This is going to be the most vicious renaming ever.

@kmsquire

This comment has been minimized.

Show comment
Hide comment
@kmsquire

kmsquire Mar 9, 2013

Member

Is there a way to do this so all external uses of ref and assign don't break immediately?

Member

kmsquire commented Mar 9, 2013

Is there a way to do this so all external uses of ref and assign don't break immediately?

@JeffBezanson

This comment has been minimized.

Show comment
Hide comment
@JeffBezanson

JeffBezanson Mar 9, 2013

Member

We could use const ref = getindex. That is frictionless but won't give a warning.

Member

JeffBezanson commented Mar 9, 2013

We could use const ref = getindex. That is frictionless but won't give a warning.

@kmsquire

This comment has been minimized.

Show comment
Hide comment
@kmsquire

kmsquire Mar 9, 2013

Member

How about a special deprecate macro for ref and assign, and broadcasting really loudly (ALL CAPS) on the mailing list? ;-)

I think it's easy enough to find all uses of ref and assign in base and packages is pretty doable. It's user's private code that I'm concerned about. But I definitely support the chance, and if it's going to happen, the earlier, the better.

Member

kmsquire commented Mar 9, 2013

How about a special deprecate macro for ref and assign, and broadcasting really loudly (ALL CAPS) on the mailing list? ;-)

I think it's easy enough to find all uses of ref and assign in base and packages is pretty doable. It's user's private code that I'm concerned about. But I definitely support the chance, and if it's going to happen, the earlier, the better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment