Make AdvancedNew iter more 0-d aware #3104

seberg · 2013-03-01T20:10:04Z

This is the old PR basically reincarnated, with some (actually relatively few) changes.

This changes the AdvancedIter to use ndim == 0 when the iteration is 0-D. This means that unfortunately a few special need to be added, since before ndim was set to 1 for the scalar case. The approach here is to store one "axisdata" as if iteration was 1-d, and add special cases to fill and use this where necessary. I do not claim that it is the best approach, it may not be...

Since it does make some sense (and einsum "tries" to basicaly) to force 0-d iterations, the oa_ndim logic is changed to allow forcing a 0-D iteration if op_axes is not NULL. This means that op_axes not being NULL is important for signalling what should happen (is this ok?), and what was previously an error now makes a forced 0-d iteration. Since that seems not too great, -1 is used to signal the old 0 cleanly, but backwards compatibility doesn't allow deprecating that.

These changes enable to remove the 0-d nditer hack, and make einsum work for the 0-d special case without modification.

charris · 2013-03-01T22:29:50Z

@mwiebe Might be useful for Mark to comment.

seberg · 2013-03-01T22:49:15Z

Maybe for the first half gh-2840 is better, much less intrusive and 0-d does not fit too well in anyway (though I would move the NDIM reporting fix from npyiter_pywrap to npyiter_api.c probably). That is independent from the oa_ndim changes.

mwiebe · 2013-03-01T23:52:52Z

numpy/core/tests/test_nditer.py

+def test_0d_iter():
+    # Basic test for iteration of 0-d arrays:
+    i = nditer([2, 3], ['multi_index'], [['readonly']]*2)
+    assert_(i.ndim == 0)


Small style comment - wouldn't using assert_equal be better, so if something does fail in the future, it will print out the values that are different?

mwiebe · 2013-03-01T23:57:17Z

The changes look good to me, I see there were quite a bit of small changes required where the code was using the invariant that ndim >= 1. I've built and run the tests, and I get two failures, one of which looks to be related to this change. Are you getting the same thing? I'll run the tests without the patch to check that.

======================================================================
FAIL: Test numpy dot with different order C, F
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Anaconda\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "C:\Anaconda\lib\site-packages\numpy\core\tests\test_blasdot.py", line 149, in test_dot_array_order
    assert_almost_equal(c.T.dot(b.T).T, b.dot(c), decimal=30)
  File "C:\Anaconda\lib\site-packages\numpy\testing\utils.py", line 452, in assert_almost_equal
    return assert_array_almost_equal(actual, desired, decimal, err_msg)
  File "C:\Anaconda\lib\site-packages\numpy\testing\utils.py", line 812, in assert_array_almost_equal
    header=('Arrays are not almost equal to %d decimals' % decimal))
  File "C:\Anaconda\lib\site-packages\numpy\testing\utils.py", line 645, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not almost equal to 30 decimals

(mismatch 5.71428571429%)
 x: array([[ 0.13969034, -2.54716229,  1.43523693,  0.5850119 , -0.72933638,
         1.76177382, -0.70778686],
       [ 1.74560761,  3.13904762, -1.83863223,  0.6032992 ,  2.31215143,...
 y: array([[ 0.13969034, -2.54716229,  1.43523693,  0.5850119 , -0.72933638,
         1.76177382, -0.70778686],
       [ 1.74560761,  3.13904762, -1.83863223,  0.6032992 ,  2.31215143,...

======================================================================
FAIL: test_iterator.test_iter_array_cast
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\Anaconda\lib\site-packages\nose\case.py", line 197, in runTest
    self.test(*self.arg)
  File "C:\Anaconda\lib\site-packages\numpy\core\tests\test_iterator.py", line 836, in test_iter_array_cast
    assert_equal(i.operands[0].strides, (-96,8,-32))
  File "C:\Anaconda\lib\site-packages\numpy\testing\utils.py", line 252, in assert_equal
    assert_equal(actual[k], desired[k], 'item=%r\n%s' % (k,err_msg), verbose)
  File "C:\Anaconda\lib\site-packages\numpy\testing\utils.py", line 314, in assert_equal
    raise AssertionError(msg)
AssertionError: 
Items are not equal:
item=0

 ACTUAL: 96L
 DESIRED: -96

----------------------------------------------------------------------
Ran 4369 tests in 69.416s

mwiebe · 2013-03-02T00:03:21Z

The failures aren't from this pull request, looks like they got into master earlier. This is on windows, by the way, I suspect the tests aren't getting regularly exercised on this platform.

seberg · 2013-03-02T00:14:12Z

Thanks for checking! Probably true about windows being tested, but I believe the first test is because the lapack tests are apparently still a bit strict after making dot aware of fortran order array (there must be an open issue for macs about this), and the second test looks like a test file that does not exist any more (and thus hopefully is not a real failure).

mwiebe · 2013-03-02T00:30:51Z

Right, after deleting numpy and reinstalling, those errors aren't there. Sorry for the false alarm on that!

njsmith · 2013-03-02T11:51:08Z

numpy/core/src/multiarray/nditer_constr.c

+     * third party software must use `oa_ndim == 0` for <1.8 compatibility.
+     */
+    if ((oa_ndim == 0) && (op_axes == NULL)) {
+        oa_ndim = -1;


Why not just DEPRECATE this here and now?

@njsmith, because DEPRECATE spits out warnings. And you cannot change to -1 if you want to support <1.8 (it is not well defined for those versions). So effectively I think that third party code cannot really use -1 yet and thus deprecating it before we can say that 1.8. is old seemes not right...

Deprecation warnings are just to give people a heads up, and only visible
if explicitly requested. The functionality is deprecated; we might as
well start giving people that heads up in the standard way now, and they
can figure out for themselves when they want to make the change.
On 2 Mar 2013 14:44, "seberg" notifications@github.com wrote:

In numpy/core/src/multiarray/nditer_constr.c:

@@ -158,6 +156,16 @@
return NULL;
}

/*

\* Before 1.8, if `oa_ndim == 0`, this meant `op_axes != NULL` was an error.

\* With 1.8, `oa_ndim == -1` takes this role, while op_axes in that case

\* enforces a 0-d iterator. At some point this should be deprecated, but

\* third party software must use `oa_ndim == 0` for <1.8 compatibility.

*/

if ((oa_ndim == 0) && (op_axes == NULL)) {

oa_ndim = -1;

@njsmith https://github.com/njsmith, because DEPRECATE spits out
warnings. And you cannot change to -1 if you want to support <1.8 (it is
not well defined for those versions). So effectively I think that third
party code cannot really use -1 yet and thus deprecating it before we can
say that 1.8. is old seemes not right...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3104/files#r3215077
.

I think this is new functionality that is not backward compatible, so the old needs to be left in. Might be worth leaving a comment tag in the code though so we can find it again.

@charris: Not sure what you're suggesting. In case it was unclear, what i was suggesting is: we keep in backwards compatibility code for the oa_ndim == 0 && op_axes == NULL special case; if anyone takes advantage of this special case we issue a DeprecationWarning; someday off in the future, once people have had time to fix their code, we take out the special case.

@rgommers Let's get a fourth opinion ;)

Each option has a downside. If I understand correctly, this is the trade-off:

Deprecate now: people who don't want to see warnings have to write code like:

if np.__version__ < '1.8.0': # OT: we need proper version tuples asap, this will break for numpy 1.10 oa_ndim = 0 else: oa_ndim = -1

Deprecate later: more people write oa_ndim = 0 now. For those that don't care about earlier versions, they're still OK but may have to fix their code several versions down the road. Then they can simply fix it without doing version comparisons.

I have a preference for deprecating later. Deprecation warnings aren't common for numpy (check the release notes of recent releases - 1.7.0 is the only one that has a significant amount), so I don't really agree that warnings that can't be fixed are "very common".

Completely missed that it actually it is possible to work around by using the the MultiNew iterator function in that case. And that does not even sound bad. So I would have no problem with deprecating right away.

Ah, unless you want control over the buffer size... but probably that is getting so rare that its no use to worry.

Great, never mind my comment then.

charris · 2013-03-02T17:22:25Z

@seberg, I think Mark's style comment on the test makes sense. Apart from that I think this can be merged.

seberg · 2013-03-02T18:06:53Z

Jup, fixed and rebased.

charris · 2013-03-02T18:31:11Z

Oops ;)

seberg · 2013-03-02T18:40:57Z

Would be a shame if it would work the first try, wouldn't it? :)

charris · 2013-03-02T19:01:11Z

numpy/core/src/multiarray/nditer_constr.c

+                                "contained invalid "
+                                "values %d", (int)iop, (int)i);
+                        return 0;
+                    } else if(axes_dupcheck[i] == 1) {


Great, I get to style nit pick ;) else if should be on next line.

charris · 2013-03-02T19:03:14Z

Those code comments are nice!

There are relatively few changes necessare here. However there is a conceptionally there are no axes for the 0-d case, and no axesdata needs to be used. This still uses the first axisdata. Which means that in a few places ndim == 0 is special cased or special cased to act like ndim == 1. It would probably be a little cleaner to to use the base pointers directly in the 0-d case and no axes iteration at all. That would require similar special cases though. This also makes oa_ndim == -1 the "correct" way to signal that no op_axes are given with oa_ndim == 0 being, for the time being, dual use. Either meaning that nothing may be given, or if something something was given enforcing a 0-d iteration. The necessary changes to the ufunc machinery are also done. Documented that the dtype transfer functions do not handle the scalar case unless even shape is set.

Also uses oa_ndim == -1 to signal no op_axes were given. This is slightly cleaner inside pywrap itself and is a cleaner signal for the iterator.

seberg · 2013-03-03T11:59:44Z

OK, I put in the actual deprecation. There are no tests (Where would I add them? I would need to write some C-code to test.), but I involuntarily tested the deprecation because I actually had forgotten to put in the fix for the ufuncs AdvancedNew usage, so good point for deprecating sooner rather then later ;)

seberg · 2013-03-03T12:22:15Z

If someone thinks that 3rd party code may want to use buffersize, since creating special cases in C-code for which numpy version you find is likely annoying (not sure how hard it is), I am be very much open to undoing that.

charris · 2013-04-01T19:15:40Z

@seberg What has changed about buffersize? Here I was all ready to put this in but your comment made me hesitate.

seberg · 2013-04-01T19:24:49Z

@charris nothing. That comment was about third party code using a non-default buffersize. If third party code does that but does not use op_axes (if they do, my guess is they got the same 0-d problem as einsum and like the change), they will have to check for the numpy version in C code to avoid the deprecation warning.

charris · 2013-04-01T19:53:32Z

Well then, in it goes!

Make AdvancedNew iter more 0-d aware

mwiebe reviewed Mar 1, 2013
View reviewed changes

njsmith reviewed Mar 2, 2013
View reviewed changes

This was referenced Mar 2, 2013

Pull request #445, rebased #2840

Closed

Fix index_tricks issue #445

Closed

charris reviewed Mar 2, 2013
View reviewed changes

seberg added 8 commits March 3, 2013 12:55

ENH: Allow np.nditer to support scalar op_axes

acce195

Also uses oa_ndim == -1 to signal no op_axes were given. This is slightly cleaner inside pywrap itself and is a cleaner signal for the iterator.

TST: Add basic tests for 0-d np.nditer/np.nested_iter support

c11fa49

TST: Add test that einsum multiplies scalars fine

610faef

MAINT: Remove np.ndindex 0-d hack.

22e1fe4

DOC: Add documentation clarifying the use of oa_ndim

b66a15e

DOC: mention AdvancedNew iterator change in the release notes.

20f44ef

MAINT: Remove 0-d iterator special case from ufunc_object.c

aa4d003

charris added a commit that referenced this pull request Apr 1, 2013

Merge pull request #3104 from seberg/nditer-allow-0d

d3edb4e

Make AdvancedNew iter more 0-d aware

charris merged commit d3edb4e into numpy:master Apr 1, 2013

seberg deleted the nditer-allow-0d branch April 1, 2013 19:59

This was referenced Apr 2, 2013

Einsum does not work for multiple scalar inputs #3011

Closed

DEP in 1.8.: nditer oa_ndim == 0 for no op_axes. #3189

Closed

seberg mentioned this pull request Dec 20, 2013

1.8 deprecations: Follow-up ticket #4131

Closed

6 tasks

FrancescAlted mentioned this pull request Jan 23, 2014

DeprecationWarning when using oa_dim=0 and op_axes=NULL in NumPy 1.8 pydata/numexpr#127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make AdvancedNew iter more 0-d aware #3104

Make AdvancedNew iter more 0-d aware #3104

seberg commented Mar 1, 2013

charris commented Mar 1, 2013

seberg commented Mar 1, 2013

mwiebe Mar 1, 2013

mwiebe commented Mar 1, 2013

mwiebe commented Mar 2, 2013

seberg commented Mar 2, 2013

mwiebe commented Mar 2, 2013

njsmith Mar 2, 2013

seberg Mar 2, 2013

njsmith Mar 2, 2013

charris Mar 2, 2013

njsmith Mar 2, 2013

charris Mar 3, 2013

rgommers Mar 3, 2013

seberg Mar 3, 2013

seberg Mar 3, 2013

rgommers Mar 3, 2013

charris commented Mar 2, 2013

seberg commented Mar 2, 2013

charris commented Mar 2, 2013

seberg commented Mar 2, 2013

charris Mar 2, 2013

charris commented Mar 2, 2013

seberg commented Mar 3, 2013

seberg commented Mar 3, 2013

charris commented Apr 1, 2013

seberg commented Apr 1, 2013

charris commented Apr 1, 2013

Make AdvancedNew iter more 0-d aware #3104

Make AdvancedNew iter more 0-d aware #3104

Conversation

seberg commented Mar 1, 2013

charris commented Mar 1, 2013

seberg commented Mar 1, 2013

Choose a reason for hiding this comment

mwiebe commented Mar 1, 2013

mwiebe commented Mar 2, 2013

seberg commented Mar 2, 2013

mwiebe commented Mar 2, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Mar 2, 2013

seberg commented Mar 2, 2013

charris commented Mar 2, 2013

seberg commented Mar 2, 2013

Choose a reason for hiding this comment

charris commented Mar 2, 2013

seberg commented Mar 3, 2013

seberg commented Mar 3, 2013

charris commented Apr 1, 2013

seberg commented Apr 1, 2013

charris commented Apr 1, 2013