CLN Slight refactoring to use deref less #12
Conversation
jjerphan
left a comment
jjerphan
left a comment
There was a problem hiding this comment.
Thank for the suggestions, @thomasjpfan.
I would have also appreciated to work for with less pointers to have it more readable.
Unfortunately for performance, I think it might be detrimental for the reasons given in my comments.
| np.npy_intp size = deref(vect_ptr).size() | ||
| np.ndarray arr = np.PyArray_SimpleNewFromData(1, &size, typenum, | ||
| deref(vect_ptr).data()) | ||
| vector_DITYPE_t vect = deref(vect_ptr) |
There was a problem hiding this comment.
Normally, a copy would be created for the rvalue deref(vect_ptr) here via the default copy constructor of vector_DITYPE_t. Using a (const) reference would allow to use the value without copy, but it's not possible in Cython unfortunately. I've tried to change it to:
vector_DITYPE_t vect& = deref(vect_ptr)and I get:
Error compiling Cython file:
------------------------------------------------------------
...
A StdVectorSentinel is registered as the base object for the numpy array,
freeing the C++ vector it encapsulates when the numpy array is freed.
"""
typenum = DTYPECODE if vector_DITYPE_t is vector[DTYPE_t] else ITYPECODE
cdef:
vector_DITYPE_t& vect = deref(vect_ptr)
^
------------------------------------------------------------
sklearn/metrics/_pairwise_distances_reduction.pyx:121:23: C++ references cannot be declared; use a pointer instead
Hence, I think it would be be better to revert this patch here or find a way to have a copy-less declaration (I can't see any).
What do you think?
There was a problem hiding this comment.
Ah I see. I mixed up Python semantics with C++ semantics when it comes to assignment.
With that in mind, I do not see a better way compared to what you have.
| """Coerce a std::vector of std::vector to a ndarray of ndarray.""" | ||
| cdef: | ||
| ITYPE_t n = deref(vecs).size() | ||
| vector_vector_DITYPE_t vects = deref(vects_ptr) |
There was a problem hiding this comment.
Same comment here: I think we hardly can prevent a copy if we want to define a local vects.
2 participants
This PR has minor nits around
StdVectorSentineland style choices to reduce the use ofderef.