Repository navigation
Conversation
|
Hi there @mwcraig 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. I noticed the following issues with this pull request:
Would it be possible to fix these? Thanks! If there are any issues with this message, please report them here. |
| for idx, mmap, arr in mmaps: | ||
| if mmap is not None: | ||
| arr.data = self[idx].data.data | ||
| arr = self[idx].data |
There was a problem hiding this comment.
This change does not look right, as it just changes what arr is pointing to - which is not used elsewhere in the file.
But given that making this effectively "do nothing" allows tests to pass again, I wonder if we're not better off just removing the whole stanza - although it does warn this could lead to segmentation faults for users. The only thing I wonder is whether we cannot check from the reference count whether there are other links to it, and, if so, just refuse to resize.
@embray - sorry to ping, but would you have suggestions?
There was a problem hiding this comment.
Agreed; near as I could tell arr.data was modified in case the user had a reference to arr hanging around somewhere.
There was a problem hiding this comment.
Yeah, this will basically break the purpose of this code in the first place (in which case it might as well be deleted entirely).
On one hand, the existing code is maybe trying to be too clever; on the other hand it is solving a real problem. It would be less of a problem of there were some way to "close" a Numpy array, such that a reasonable exception would be raised if trying to access data from an array whose underlying buffer no longer exists. That would be a good Numpy feature. But in the meantime it's actually IMO fine to replace an ndarray's buffer with a different buffer of the same length.
I still don't know why Numpy has decided to deprecate this, but if they insist then I still think a little helper function written in Cython to update an ndarray object's buffer is in order...
|
I also added a little rant about this over at numpy/numpy#7093 Having seen the rationale for the deprecation I agree with why it was done, but I also believe there needs to be some better solutions for buffer management, and/or tracking relationships between a given ndarray and all the view of that array (given a view you can always find the underlying array through |
This PR fixes #5797
A couple of important caveats:
The tests that were xfailed in #5797 are fixed with this change and no other tests fail, for what that is worth...