8000 BUG: retain writeable flag when indexing subclasses by juliantaylor · Pull Request #4843 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: retain writeable flag when indexing subclasses #4843

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 6, 2014
Merged

BUG: retain writeable flag when indexing subclasses #4843

merged 1 commit into from
Jul 6, 2014

Conversation

juliantaylor
Copy link
Contributor

No description provided.

@juliantaylor
Copy link
Contributor Author

fixes failures of pyfits with numpy 1.9, see astropy/astropy#2693

@seberg are there more indexing cases where this could be broken?
8000

charris
charris reviewed Jul 6, 2014
View reviewed changes
numpy/core/tests/test_indexing.py
ind = np.array([0, 1])
assert_(d[ind].flags.writeable)
assert_(d[...].flags.writeable)
assert_(d[True].flags.writeable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The d[True] raises a DeprecationWarning that causes the test to fail.

/home/charris/.local/lib/python2.7/site-packages/numpy/core/records.py:459: DeprecationWarning: using a boolean instead of an integer will result in an error in the future

The easy fix is to remove the check or use an integer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

charris added a commit that referenced this pull request Jul 6, 2014
@charris
Merge pull request #4843 from juliantaylor/subclass-writeable
94417e4 
BUG: retain writeable flag when indexing subclasses
@charris charris merged commit 94417e4 into numpy:master Jul 6, 2014
@charris
Copy link
Member
charris commented Jul 6, 2014

Thanks Julian. I think we are about ready for the second beta. Are there any other PR's that you want to get in?

@juliantaylor
Copy link
Contributor Author

I think #4840 would still be good

@juliantaylor juliantaylor deleted the subclass-writeable branch July 6, 2014 16:51
@juliantaylor
Copy link
Contributor Author

I'd also like to know whats going on in #4836 but I can't reproduce it, I guess I could install a intel compiler ..

@charris
Copy link
Member
charris commented Jul 6, 2014

There are still three numpy related errors in sklearn 0.14, but they are fixed in 0.15.0b2, so I think we are OK there.

@seberg
Copy link
Member
seberg commented Jul 6, 2014

Sorry, was travelling a bit today, as noted in the other PR, I think this would be better:

--- a/numpy/core/src/multiarray/mapping.c
+++ b/numpy/core/src/multiarray/mapping.c
@@ -1569,7 +1569,7 @@ array_subscript(PyArrayObject *self, PyObject *op)
                                       PyArray_SHAPE(tmp_arr),
                                       PyArray_STRIDES(tmp_arr),
                                       PyArray_BYTES(tmp_arr),
-                                      0, /* TODO: Flags? */
+                                      PyArray_FLAGS(self),
                                       (PyObject *)self);

@charris
Copy link
Member
charris commented Jul 6, 2014

@seberg

I reverted this and applied your patch for testing. It fails one of Julians tests:

In [12]: d = np.rec.array([('NGC1001', 11), ('NGC1002', 1.), ('NGC1003', 1.)],
                         dtype=[('target', 'S20'), ('V_mag', '>f4')])

In [13]: ind = np.array([False,  True,  True], dtype=bool)

In [14]: d[ind].flags
Out[14]: 
  C_CONTIGUOUS : True
  F_CONTIGUOUS : True
  OWNDATA : False
  WRITEABLE : False
  ALIGNED : False
  UPDATEIFCOPY : False

It works for the integer indexing, but the boolean case looks to be a problem.

@juliantaylor
Copy link
Contributor Author

you need to apply it in both places I applied my fix too, don't know why I didn't see that ._.

@charris
Copy link
Member
charris commented Jul 6, 2014

@juliantaylor I step outside and you beat me to it ;)

@charris
Copy link
Member
charris commented Jul 6, 2014

Applying this

diff --git a/numpy/core/src/multiarray/mapping.c b/numpy/core/src/multiarray/map
index e2f3b50..bc94bd0 100644
--- a/numpy/core/src/multiarray/mapping.c
+++ b/numpy/core/src/multiarray/mapping.c
@@ -1047,7 +1047,7 @@ array_boolean_subscript(PyArrayObject *self,
         Py_INCREF(dtype);
         ret = (PyArrayObject *)PyArray_NewFromDescr(Py_TYPE(self), dtype, 1,
                             &size, PyArray_STRIDES(ret), PyArray_BYTES(ret),
-                            0, (PyObject *)self);
+                            PyArray_FLAGS(self), (PyObject *)self);

         if (ret == NULL) {
             Py_DECREF(tmp);
@@ -1569,7 +1569,7 @@ array_subscript(PyArrayObject *self, PyObject *op)
                                       PyArray_SHAPE(tmp_arr),
                                       PyArray_STRIDES(tmp_arr),
                                       PyArray_BYTES(tmp_arr),
-                                      0, /* TODO: Flags? */
+                                      PyArray_FLAGS(self),
                                       (PyObject *)self);

         if (result == NULL) {

Does fix it. Should I make a PR for this? Seems right to me.

@seberg
Copy link
Member
seberg commented Jul 6, 2014

Hmmm, probably coul duse a goto_wrap_array in the boolean case too, instead of doing it by hand inside the boolean assignment code. A bit more elegant, I didn't expect it necessary in both cases ;).

But yes, seems right to me too.

@juliantaylor
Copy link
Contributor Author

#4847

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@juliantaylor @charris @seberg
0