Added inplace_increment function #326

jsalvatier · 2012-06-27T23:45:54Z

I've added an inplace_increment function which allows you to increment an array inplace using advanced indexing. The key feature is that repeated indexes will repeatedly increment the array whereas

a[idx] +=1

will not do this. More discussion here and here.

I've included, documentation, a couple of tests and supported multiple datatypes.

I'm new to doing Python functions in C so a good once over of the C code is a good idea. I'm also new to submitting code for numpy, so I've probably made some mistakes somewhere.

…kind, it will copy, otherwise it will just do it inplace

njsmith · 2012-07-06T16:21:53Z

I have mixed feelings about this. The functionality is often-requested and clearly useful, but the implementation has a number of problems. The basic features numpy provides are indexing and generic operations over generic types. This code uses numpy's indexing, but ignores the generic operation and type support, so it only supports one operation (addition) and a fixed set of types. All that would be fine if it were just implementation limitations that could be fixed later, but the API is not flexible enough to support other operations -- a "proper" version of this functionality would probably be a method on ufuncs, not a standalone function. So the API is kind of a dead-end as well.

I don't want to make the perfect the enemy of the good, but on balance I think there's enough wrong with this that I'm -0.5 on merging it and making it a permanent part of the numpy API. The best compromise might be to extract the core function and release it as a tiny standalone library? It looks like it only uses public numpy API functions, so it should be easy to do. That way people who want the feature have an easy way to get it until numpy grows a cleaner solution.

jsalvatier · 2012-07-06T16:41:37Z

I'd be in favor of a stand alone library (it was my original intention to put this into a separate package) except that the MapIter interface is unfortunately not public; mapping.c has a comment that explains:

/*

The mapiter object must be created new each time. It does not work
to bind to a new array, and continue.
*
This was the orginal intention, but currently that does not work.
Do not expose the MapIter_Type to Python.
*
It's not very useful anyway, since mapiter(indexobj); mapiter.bind(a);
mapiter is equivalent to a[indexobj].flat but the latter gets to use
slice syntax.
*/

If MapIter could be exposed, then I'd be happy to make this a function in a separate package. I don't know whether it would be difficult to fix the reuse issue. I think Mark Wiebe was the person who implemented MapIter.

I'll look into making this a function of binary ufuncs like reduce, but I suspect that's still above my skill level.

jsalvatier · 2012-07-06T17:23:53Z

Actually looking at the implementation of PyUFunc_ReduceWrapper, it doesn't look like it would be too difficult to do something similar for this. However, I'm having a hard time picturing how the added generality would be used. Can you think of some useful cases where this would be used on ufuncs besides add? I can imagine it being useful on prod and pow, but I can't think of any concrete uses.

njsmith · 2012-07-06T18:32:24Z

I don't have any concrete cases in mind for using this with prod and pow, but I bet someone will show up with one sooner or later... automatically supporting all numpy dtypes is the more useful part, I think.

If MapIter isn't exposed then that'll be a hurdle, though, because ufuncs (which live in the "umath" library) don't have access to the "multiarray" library internal functions. @mwiebe, is there any reasonable way to do ndarray indexing via the public C API?

teoliphant · 2012-07-17T20:13:19Z

While in an ideal world, perhaps this functionality would be on ufuncs. However, that is actually quite a bit of work. The fancy-indexing feature is just not exposed to the ufunc machinery. Thus, I think that currently this patch does a pretty good job as is. I don't think it would be too difficult to add a verison for mul and pow using a similar API at some point.

teoliphant · 2012-07-17T20:15:08Z

numpy/core/bscript

@@ -428,6 +428,7 @@ def pre_build(context):
                "src/multiarray/arraytypes.c.src",
                "src/multiarray/nditer_templ.c.src",
                "src/multiarray/lowlevel_strided_loops.c.src",
+				"src/multiarray/mapping.c.src",


Indentation problem.

jsalvatier · 2012-07-18T16:52:48Z

I think I've fixed those formatting issues.

charris · 2012-07-19T01:09:51Z

numpy/core/src/multiarray/mapping.c.src

+    inplace_binop add_inplace = NULL; 
+    int type_number = PyArray_TYPE(a);
+    int i =0;
+    while  (type_numbers[i] >= 0 && addition_funcs[i] != NULL)


wrong curly braces style for while and if.

hgomersall · 2012-07-19T09:06:01Z

numpy/core/src/multiarray/mapping.c.src

+
+/**begin repeat
+* #type  = npy_complex64, npy_complex128#
+* #typen = NPY_COMPLEX64, NPY_COMPLEX128#


As for the reals above, what happens for the longer complex types?

…omplex now...

…ind of complex now..." This reverts commit 684ac6a.

travisbot · 2012-07-23T16:51:06Z

This pull request fails (merged 16603b5 into f2f0ac0).

travisbot · 2012-07-23T18:28:15Z

This pull request fails (merged 6a80aba into f2f0ac0).

jsalvatier · 2012-07-23T18:28:29Z

I read through the C style document and went through and tried to make this patch conform.

I've also added support for all int,uint, float, complex datatype bit-width sizes that are listed at the bottom of here: http://docs.scipy.org/doc/numpy/reference/c-api.dtype.html

bfroehle · 2012-07-25T00:37:58Z

numpy/core/src/multiarray/mapping.c.src

+            &inc)) { 
+        return NULL;
+    }
+    if (arg_a->ob_type != &PyArray_Type){


I think you meant to write if (!PyArray_Check(arg_a)) {.

Thanks, I was looking for that function.

… two error checks

travisbot · 2012-07-25T17:52:21Z

This pull request fails (merged 73b36f4 into f2f0ac0).

njsmith · 2012-07-26T11:30:59Z

It's great that you guys are interested in this, but I'm worried that you're spending a lot of time in ways that aren't really getting the patch closer to merging. The main obstacle is still the points that I raised. IMHO the best way forward would be to work out how to expose an API for doing indexing:
http://mail.scipy.org/pipermail/numpy-discussion/2012-July/063411.html

Of course there may be other options as well, but this approach doesn't seem too difficult and AFAICT everyone seems to like it.

jsalvatier · 2012-07-26T13:56:34Z

What would be necessary to do that?
On Jul 26, 2012 4:31 AM, "njsmith" <
reply@reply.github.com>
wrote:

It's great that you guys are interested in this, but I'm worried that
you're spending a lot of time in ways that aren't really getting the patch
closer to merging. The main obstacle is still the points that I raised.
IMHO the best way forward would be to work out how to expose an API for
doing indexing:
http://mail.scipy.org/pipermail/numpy-discussion/2012-July/063411.html

Of course there may be other options as well, but this approach doesn't
seem too difficult and AFAICT everyone seems to like it.

Reply to this email directly or view it on GitHub:
#326 (comment)

teoliphant · 2012-07-26T14:36:53Z

Exposing an API for indexing would be a good start. Basically, expose the bound MapIter functions that you used (MapIter_Next, etc.)

Then, this API could be used to add in-place indexing as a method to ufuncs more easily (re-using the inner loops of those ufuncs to do the actual calculation).

-Travis

On Jul 26, 2012, at 8:56 AM, john salvatier wrote:

What would be necessary to do that?
On Jul 26, 2012 4:31 AM, "njsmith" <
reply@reply.github.com>
wrote:

It's great that you guys are interested in this, but I'm worried that
you're spending a lot of time in ways that aren't really getting the patch
closer to merging. The main obstacle is still the points that I raised.
IMHO the best way forward would be to work out how to expose an API for
doing indexing:
http://mail.scipy.org/pipermail/numpy-discussion/2012-July/063411.html

Of course there may be other options as well, but this approach doesn't
seem too difficult and AFAICT everyone seems to like it.

Reply to this email directly or view it on GitHub:
#326 (comment)

Reply to this email directly or view it on GitHub:
#326 (comment)

jsalvatier · 2012-07-26T15:35:13Z

Can the API be exposed as-is or do we need to solve the problem of
resetting the MapIterObject? Where do you say which functions are exposed
as interface?

On Thu, Jul 26, 2012 at 7:36 AM, Travis E. Oliphant <
reply@reply.github.com

wrote:

Exposing an API for indexing would be a good start. Basically, expose
the bound MapIter functions that you used (MapIter_Next, etc.)

Then, this API could be used to add in-place indexing as a method to
ufuncs more easily (re-using the inner loops of those ufuncs to do the
actual calculation).

-Travis

On Jul 26, 2012, at 8:56 AM, john salvatier wrote:

What would be necessary to do that?
On Jul 26, 2012 4:31 AM, "njsmith" <
reply@reply.github.com>
wrote:

It's great that you guys are interested in this, but I'm worried that
you're spending a lot of time in ways that aren't really getting the
patch
closer to merging. The main obstacle is still the points that I raised.
IMHO the best way forward would be to work out how to expose an API for
doing indexing:
http://mail.scipy.org/pipermail/numpy-discussion/2012-July/063411.html

Of course there may be other options as well, but this approach doesn't
seem too difficult and AFAICT everyone seems to like it.

Reply to this email directly or view it on GitHub:
#326 (comment)

Reply to this email directly or view it on GitHub:
#326 (comment)

Reply to this email directly or view it on GitHub:
#326 (comment)

njsmith · 2012-07-26T16:11:56Z

There's a script that runs over the source and generates the actual API proper. To mark a function to be exported, you just put a magic comment that says NUMPY_API in front of it. If you grep the source you'll find some examples to follow. Then you should also add it to the list in numpy/core/code_generators/numpy_api.py.

I'm not familiar enough with the details of the MapIterObject interface to comment off-the-cuff on whether it can be exposed as is. If no-one else speaks up, though, then you can always make a first hackish attempt and post it for review. (Note that Travis did have a suggestion about how the interface should look in that comment I posted up above, though.) I think the solution to the resetting problem is to just not expose any functionality that would let people run into the problem, though I don't know what the problem is in detail so I might be missing something.

jsalvatier · 2012-08-06T00:26:35Z

I've made a separate pull request, for njsmith's recommendation to expose the MapIter API (#375).

jsalvatier · 2012-08-07T01:55:40Z

I've made a cleaned up pull request (#377).

jsalvatier · 2013-02-08T19:06:36Z

We decided not to move ahead with this pull request and instead just added API support: #377

refactor: Reduce vmlsl_* register usage

jsalvatier added 17 commits June 25, 2012 14:40

added index increment function

2d4fc32

made index_increment return the array; if the input wasn't the right …

ef1d021

…kind, it will copy, otherwise it will just do it inplace

made index increment require a double array

6af27c5

made index increment require a double array

82587d3

improved error handling

e9b0c2b

added support for multiple datatypes

ccc1189

only goto fail if we've initialized mit

745faa8

added initial doc

5aef9c7

renamed index_increment to inplace_increment

86b15d6

made inplace_increment import by default

a47e934

updated examples

77d5341

supported complex types

e6cecf9

added initial tests

47b1f15

added more tests

27bd77b

fixed tests mostly

420a241

fixed boolean test

3085f98

added another example

7d03753

teoliphant reviewed Jul 17, 2012
View reviewed changes

fixed formatting issues that travis pointed out

6f84384

charris reviewed Jul 19, 2012
View reviewed changes

hgomersall reviewed Jul 19, 2012
View reviewed changes

jsalvatier added 5 commits July 22, 2012 22:05

fixed style issues

68cafc1

added more types and refactored typing generation; it seems kind of c…

684ac6a

…omplex now...

corrected a segfault issue from reformatting

16603b5

Revert "added more types and refactored typing generation; it seems k…

bd513bf

…ind of complex now..." This reverts commit 684ac6a.

forgot ;

f9314aa

jsalvatier added 2 commits July 23, 2012 10:06

added other datatypes

44a1c49

added check for non-array

6a80aba

bfroehle reviewed Jul 25, 2012
View reviewed changes

moved the looping into the datatype specific functions and simplified…

73b36f4

… two error checks

jsalvatier mentioned this pull request Sep 26, 2012

MapIter API #377

Closed

nouiz mentioned this pull request Oct 29, 2012

Implement GpuAdvancedIncSubtensor Theano/Theano#930

Closed

Merge remote-tracking branch 'main/master'

d862f52

jsalvatier mentioned this pull request Dec 14, 2012

Full advanced Indexing support + gradient Theano/Theano#1083

Closed

jsalvatier closed this Feb 8, 2013

luyahan pushed a commit to plctlab/numpy that referenced this pull request Apr 25, 2024

Merge pull request numpy#326 from howjmay/vmlsl_reg

4b0fb91

refactor: Reduce vmlsl_* register usage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added inplace_increment function #326

Added inplace_increment function #326

jsalvatier commented Jun 27, 2012

njsmith commented Jul 6, 2012

jsalvatier commented Jul 6, 2012

jsalvatier commented Jul 6, 2012

njsmith commented Jul 6, 2012

teoliphant commented Jul 17, 2012

teoliphant Jul 17, 2012

jsalvatier commented Jul 18, 2012

charris Jul 19, 2012

hgomersall Jul 19, 2012

travisbot commented Jul 23, 2012

travisbot commented Jul 23, 2012

jsalvatier commented Jul 23, 2012

bfroehle Jul 25, 2012

jsalvatier Jul 25, 2012

travisbot commented Jul 25, 2012

njsmith commented Jul 26, 2012

jsalvatier commented Jul 26, 2012

teoliphant commented Jul 26, 2012

jsalvatier commented Jul 26, 2012

njsmith commented Jul 26, 2012

jsalvatier commented Aug 6, 2012

jsalvatier commented Aug 7, 2012

jsalvatier commented Feb 8, 2013

Added inplace_increment function #326

Added inplace_increment function #326

Conversation

jsalvatier commented Jun 27, 2012

njsmith commented Jul 6, 2012

jsalvatier commented Jul 6, 2012

jsalvatier commented Jul 6, 2012

njsmith commented Jul 6, 2012

teoliphant commented Jul 17, 2012

teoliphant Jul 17, 2012

Choose a reason for hiding this comment

jsalvatier commented Jul 18, 2012

charris Jul 19, 2012

Choose a reason for hiding this comment

hgomersall Jul 19, 2012

Choose a reason for hiding this comment

travisbot commented Jul 23, 2012

travisbot commented Jul 23, 2012

jsalvatier commented Jul 23, 2012

bfroehle Jul 25, 2012

Choose a reason for hiding this comment

jsalvatier Jul 25, 2012

Choose a reason for hiding this comment

travisbot commented Jul 25, 2012

njsmith commented Jul 26, 2012

jsalvatier commented Jul 26, 2012

teoliphant commented Jul 26, 2012

jsalvatier commented Jul 26, 2012

njsmith commented Jul 26, 2012

jsalvatier commented Aug 6, 2012

jsalvatier commented Aug 7, 2012

jsalvatier commented Feb 8, 2013