Reworking insert for Issue #452 I have come to a point where I think that some clarification is necessary to decide what insert is supposed to do exactly. And since it is related to this issue and I its not quite fixed, I will hijack this here. This is basically how things work(ed) (ignoring repetition):

# Please neglect the fact that of course `arr` is not modified but a larger version of it and
# that `obj` is changed accordingly.
# Numpy 1.6 did the equivalent of for insertion of values:
arr[:,obj,:] = values

# Numpy 1.7 does this:
arr[:,np.atleast_1d(obj),:] = values

Note that this quite a big change for handleing of scalars, because of broadcasting! For example:

>>> arr = np.arange(4).reshape(2,2)
>>> np.insert(arr, 0, [-2, -1], axis=1) # 1.6, gives this, while 1.7 gives an error.
array([[-2,  0,  1],
       [-1,  2,  3]])

Now numpy 1.7 has an added extra. If values.ndim >= arr.ndim then the insertion is repeated by len(values). This is in so far harmless as it only makes it work in cases where it would fail before. However I think the way this is done must be reconsidered, because consider this:

>>> a = np.array([[1, 1], [2, 2], [3, 3]])
>>> np.insert(a, [1], [[1],[2],[3]], axis=1)
array([[1, 1, 1],
       [2, 2, 2],
       [3, 3, 3]])
>>> np.insert(a, 1, [[1],[2],[3]], axis=1)
array([[1, 1, 1, 1, 1],
       [2, 2, 2, 2, 2],
       [3, 3, 3, 3, 3]])

This is inconsistent in itself, because add one axis to a and you get the other result:

>>> np.insert([a], 1, [[1],[2],[3]], axis=2)
array([[1, 1, 1],
       [2, 2, 2],
       [3, 3, 3]])

Now the way to make the 1.7 behavior consistent you can use values = np.array(values, copy=False, ndmin=arr.ndim) and then obj = [obj] * values.shape[axis] (which I did in my PR, and also made it possible to repeat 1D arrays). There are two options in my opinion:

1. Fix 1.7 behavior to use obj = [obj] * values.shape[axis], so that obj=0 and obj=[0] are always the same
2. Restore the special case for obj.ndim=0, allow repeating with len(values) but then np.rollaxis(values, 0, axis+1) so that the repetition is along the specified axis. The option 1. could then be added for the case of obj=[0].

In a way I think that 1. has less surprises for the users (0 and [0] do not behave completely different), but 2. does not introduce regressions and is maybe more logical. I have implemented option 1. in my PR, but for the sake of backward compatibility I would now tend a bit to option 2. I think. In ceterum censeo: Insert has a couple of other bugs that must be fixed, see my PR ;).

Btw: option 1 would mean that (I think, too long staring at this):

>>> a = np.array([[1, 1], [2, 2], [3, 3]])
>>> np.insert(a, 1, [[1],[2],[3]], axis=1) # Note, that its the scalar 1.
array([[1, 2, 3, 1, 1],
       [1, 2, 3, 2, 2],
       [1, 2, 3, 3, 3]])

To update I personally now think that option 2. from above is the only correct aproach (for consistency with arr[:,obj,:] = ...) and I have changed #452 like that, plus its what numpy 1.6 and before did. The only question is if multiple inserts with a scalar should be allowed at all (they could maybe raise an error saying to use obj=[0] instead of obj=0), because I think the difference between the two may be a bit overly confusing otherwise and repitition is much more natural for obj=[0], since there its consistent that obj=[0] does basically the same as np.concatenate((values, arr), axis=...).

Suggestion for 1.7?

Its not my decision, but personally I would suggest reverting all changes in np.insert for 1.7 if thats viable. The "Bug" fixed by 2c04244 is really more a feature request in the first place IMO and its the most minimal change that does not create regressions and inconsistencies.

If you think that multiple insertion with a scalar should be allowed and introduced in 1.7, then it would be fine to to replace:

        if isscalar(values):
            obj = [obj]
        else:
            values = asarray(values)
            if ndim > values.ndim:
                obj = [obj]
            else:
                obj = [obj] * len(values)

with:

        values = array(values, copy=False, ndmin=arr.ndim)
        values = np.rollaxis(values, 0, axis+1)
        obj = [obj] * values.shape[axis]

which restores compatbility with 1.6.

Hmm, yeah, I concur that #452 is (much) more consistent in handling the different use cases of insert. I hadn't considered the broadcasting behaviour, as well as the multidimensional slicing behaviour when implementing 8460514..

This also shows the necessity of comprehensive test cases to define function behaviour and act as safeguard against regressions; it is sometimes difficult to grok the full extent some change can have on a function.

I think the multiple insertion feature should stay, however, because it has some nice use cases, imo.
Isn't that already part of #452?

@87 yes the lack of tests really created this problem... My PR implements multiple insertion for both 0 and [0]. I am not against the multiple insertion feature. Its good. I say that multiple insertion for scalar could be omitted in favor of only multiple insertion for ie. [0] (and the error could tell the user about it) but I don't care really, often it may be convinient to use scalars even if it is a bit prone to confusion.

The deal is, 1.7 is about to be released and to my understanding my PR is too extensive a change (even if reworked on the booleans and disregarding delete) to include into it. And I would prefere to remove the new feature for the moment over introducing a regression which would have to be reverted again in the next release and for which implications in the wild are difficult to estimate. Though the fix I gave above seems minimal enough, so I guess that would likely be best. But it is in any case for the core devs to decide.

This should be fixed with no remaining regressions after my last PR linked above, which is in 1.7. (and covered by tests). This means that maybe master still has a regression here, my reasoning back then was that I thought master will probably get a larger rewrite through my other PR at some point sooner or later in any case.

Ok, I just want to make sure there's enough information here so that we can
make sure nothing gets forgotten. So 1.7 is fine, but master/1.8 may have a
regression?
On 16 Dec 2012 04:22, "seberg" notifications@github.com wrote:

This should be fixed with no remaining regressions after my last PR linked
above, which is in 1.7. (and covered by tests). This means that maybe
master still has a regression here, my reasoning back then was that I
thought master will probably get a larger rewrite through my other PR at
some point sooner or later in any case.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/378#issuecomment-11414139.

yup exactly, 1.7. is fine with gh-2697 and 1.8. as of now lacks this.

Oog, it looks like we need to forward-port gh-2697 from maintenance/1.7.x before we can release 1.8? Or is gh-452 a better fix? @seberg, do you know the status here?

@njsmith we could forward port yes, but gh-452 includes this. I personally think gh-452 should be pretty much ready and also that it should be in 1.8. That is plus/minus the question if negative indices should only be a future warning and otherwise be discarded, which is not the case right now.

gh-452 merged, so closing this.

(Though @seberg -- your last comment on this issue raised the negative indices question -- did that get resolved?)

@njsmith, yes. There are now warnings for these cases (for delete anyway, for insert I think they just work, but previously they never did anything that made sense). So if I did everything as intended, there should be no real changes yet, just warnings.

awesome

On Thu, Apr 11, 2013 at 9:59 PM, seberg notifications@github.com wrote:

@njsmith https://github.com/njsmith, yes. There are now warnings for
these cases (for delete anyway, for insert I think they just work, but
previously they never did anything that made sense). So if I did everything
as intended, there should be no real changes yet, just warnings.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/378#issuecomment-16260974
.