-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
REF: avoid getattr pattern for rank_1d, rank_2d #29137
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these definitely read a lot nicer
@@ -455,7 +455,7 @@ def _group_add(complexfloating_t[:, :] out, | |||
if len(values) != len(labels): | |||
raise ValueError("len(index) != len(labels)") | |||
|
|||
nobs = np.zeros((len(out), out.shape[1]), dtype=np.int64) | |||
nobs = np.zeros((<object>out).shape, dtype=np.int64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the object cast buy us here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://groups.google.com/forum/#!topic/cython-users/xheRGGOiuYw
For reasons unknown, out.shape
is evaluated as an 8-tuple instead of 2-tuple, and this call raises at runtime without the <object>
cast. That's the reason why the status quo doesn't use out.shape
directly on L458.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty interesting...and cast to object here doesn't trigger any underlying functions? Not sure if it just changes the type of pointer Cython uses or if it actually changes the memview
thanks @jbrockmendel yes nice cleanup |
Also fix incorrectly-typed
nobs
in libgroupby