Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MAINT: shims for array copy semantics #4482

Merged
merged 3 commits into from
Apr 21, 2024
Merged

MAINT: shims for array copy semantics #4482

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df3a033
MAINT: shims for array copy semantics
tylerjereddy Mar 2, 2024
320a940
MAINT: PR 4482 revisions
tylerjereddy Mar 12, 2024
ff93d70
MAINT: PR 4482 revisions
tylerjereddy Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions package/MDAnalysis/lib/formats/cython_util.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ cdef class ArrayWrapper:
self.data_type = data_type
self.ndim = ndim

def __array__(self):
def __array__(self, copy=None):
Copy link
Member Author

Choose a reason for hiding this comment

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

here and elswhere we should probably also add dtype=None before copy=None, similar to the revisions added in my PR upstream at: scipy/scipy#20172

It doesn't seem to matter for testing purposes at the moment, but adding the dtype=None is more formally correct now.

""" Here we use the __array__ method, that is called when numpy
tries to get an array from the object."""
ndarray = cnp.PyArray_SimpleNewFromData(self.ndim,
Expand Down Expand Up @@ -110,7 +110,11 @@ cdef cnp.ndarray ptr_to_ndarray(void* data_ptr, cnp.int64_t[:] dim, int data_typ
array_wrapper = ArrayWrapper()
array_wrapper.set_data(<void*> data_ptr, <int*> &dim[0], dim.size, data_type)

cdef cnp.ndarray ndarray = np.array(array_wrapper, copy=False)
if np.__version__[0] == "2":
copy = None
else:
copy = False
Copy link
Member Author

Choose a reason for hiding this comment

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

A more concise/idiomatic upstream approach was applied in the revised version of my patch in case we care about that: https://github.com/scipy/scipy/pull/20172/files#diff-d28fbb0be287769c763ce61b0695feb0a6ca0e67b28bafee20526b46be7aaa84R59

Basically, abstracting the check to avoid duplication and using np.lib.NumpyVersion instead of pep440 or a raw string check like here. We probably don't need to worry about 2.0.0dev0 versions though, so we may care less about that, though the version check duplication I've added more broadly is indeed a bit much.

cdef cnp.ndarray ndarray = np.array(array_wrapper, copy=copy)
# Assign our object to the 'base' of the ndarray object
ndarray[:] = array_wrapper.__array__()
# Increment the reference count, as the above assignement was done in
Expand Down
28 changes: 22 additions & 6 deletions package/MDAnalysis/lib/transformations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,8 +325,12 @@
M = np.identity(4)
M[:3, :3] = R
if point is not None:
if np.__version__[0] == "2":
copy = None

Check warning on line 329 in package/MDAnalysis/lib/transformations.py

View check run for this annotation

Codecov / codecov/patch

package/MDAnalysis/lib/transformations.py#L329 

Added line #L329 was not covered by tests
else:
copy = False
# rotation not around origin
point = np.array(point[:3], dtype=np.float64, copy=False)
point = np.array(point[:3], dtype=np.float64, copy=copy)
M[:3, 3] = point - np.dot(R, point)
return M

Expand Down Expand Up @@ -497,7 +501,11 @@

"""
M = np.identity(4)
point = np.array(point[:3], dtype=np.float64, copy=False)
if np.__version__[0] == "2":
copy = None

Check warning on line 505 in package/MDAnalysis/lib/transformations.py

View check run for this annotation

Codecov / codecov/patch

package/MDAnalysis/lib/transformations.py#L505 

Added line #L505 was not covered by tests
else:
copy = False
point = np.array(point[:3], dtype=np.float64, copy=copy)
normal = unit_vector(normal[:3])
if perspective is not None:
# perspective projection
Expand All @@ -515,7 +523,7 @@
M[3, 3] = np.dot(perspective, normal)
elif direction is not None:
# parallel projection
direction = np.array(direction[:3], dtype=np.float64, copy=False)
direction = np.array(direction[:3], dtype=np.float64, copy=copy)
scale = np.dot(direction, normal)
M[:3, :3] -= np.outer(direction, normal) / scale
M[:3, 3] = direction * (np.dot(point, normal) / scale)
Expand Down Expand Up @@ -970,8 +978,12 @@
True

"""
v0 = np.array(v0, dtype=np.float64, copy=False)[:3]
v1 = np.array(v1, dtype=np.float64, copy=False)[:3]
if np.__version__[0] == "2":
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to wrap this in a helper routine to avoid repetition eg:

 v0 = np.array(v0, dtype=np.float64, copy=no_copy_shim())[:3]


def no_copy_shim():
      if np.__version__[0] == "2":
        copy=None
     else:
        copy=False
...

Ill leave it up to you @tylerjereddy?

copy = None

Check warning on line 982 in package/MDAnalysis/lib/transformations.py

View check run for this annotation

Codecov / codecov/patch

package/MDAnalysis/lib/transformations.py#L982 

Added line #L982 was not covered by tests
else:
copy = False
v0 = np.array(v0, dtype=np.float64, copy=copy)[:3]
v1 = np.array(v1, dtype=np.float64, copy=copy)[:3]

if v0.shape != v1.shape or v0.shape[1] < 3:
raise ValueError("vector sets are of wrong shape or type")
Expand Down Expand Up @@ -1314,7 +1326,11 @@
True

"""
M = np.array(matrix, dtype=np.float64, copy=False)[:4, :4]
if np.__version__[0] == "2":
copy = None

Check warning on line 1330 in package/MDAnalysis/lib/transformations.py

View check run for this annotation

Codecov / codecov/patch

package/MDAnalysis/lib/transformations.py#L1330 

Added line #L1330 was not covered by tests
else:
copy = False
M = np.array(matrix, dtype=np.float64, copy=copy)[:4, :4]
if isprecise:
q = np.empty((4, ), dtype=np.float64)
t = np.trace(M)
Expand Down