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.

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

CLN: Get rid of PyArray_GetCastFunc #56114

Merged
merged 6 commits into from
Nov 26, 2023

Conversation

lithomas1
Copy link
Member

@lithomas1 lithomas1 commented Nov 22, 2023

@lithomas1 lithomas1 marked this pull request as ready for review November 22, 2023 16:34
@lithomas1 lithomas1 requested a review from WillAyd as a code owner November 22, 2023 16:34
@lithomas1
Copy link
Member Author

cc @seberg

@lithomas1 lithomas1 added this to the 2.2 milestone Nov 22, 2023
PyObject *scalarItem = PyArray_ToScalar(dataptr, labels);
PyArray_CastScalarToCtype(scalarItem, &i8date,
PyArray_DescrFromType(NPY_INT64));
Py_DECREF(scalarItem);
Copy link
Contributor

@seberg seberg Nov 22, 2023

Choose a reason for hiding this comment

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

Here, you can just use:

memcpy(&i8date, dataptr, sizeof(i8data));

I think, since you know this is an int64, as datetimes are int64 (I am unsure about alignment, thus the memcpy).

Same below? (at last I would think so?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Wow, this is even easier, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I just realized the cast function doesn't handle alignment, so even i8data = *(npy_int64 *)dataptr; should work.

@mroeschke mroeschke added Internals Related to non-user accessible pandas implementation Compat pandas objects compatability with Numpy or Python functions labels Nov 22, 2023
@@ -1532,7 +1521,6 @@ void Object_beginTypeContext(JSOBJ _obj, JSONTypeContext *tc) {

PyArray_Descr *outcode = PyArray_DescrFromType(NPY_INT64);
PyArray_CastScalarToCtype(obj, &longVal, outcode);
Py_DECREF(outcode);
Copy link
Member

Choose a reason for hiding this comment

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

Just confirming this was also an intentional cleanup?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, this is an accident and shouldn't be included.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch.

I wonder if we're leaking references to PyArray_Descr elsewhere than.

The numpy docs don't seem too clear about whether we're borrowing references or owning it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah the deleted code here used to leak them... One point is that array creation functions steal them, though.

@lithomas1 lithomas1 merged commit 6d1b07f into pandas-dev:main Nov 26, 2023
40 checks passed
@lithomas1 lithomas1 deleted the cln-np-deprecated branch November 26, 2023 16:46
@lithomas1
Copy link
Member Author

Merging then, thanks for the help all.

@lithomas1 lithomas1 modified the milestones: 2.2, 2.1.4 Dec 4, 2023
@lithomas1
Copy link
Member Author

@meeseeksdev backport 2.1.x.

Copy link

lumberbot-app bot commented Dec 4, 2023

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
git checkout 2.1.x
git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
git cherry-pick -x -m1 6d1b07f11b3f383bb8e4f895cfead3125e89379d
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
git commit -am 'Backport PR #56114: CLN: Get rid of PyArray_GetCastFunc'
  1. Push to a named branch:
git push YOURFORK 2.1.x:auto-backport-of-pr-56114-on-2.1.x
  1. Create a PR against branch 2.1.x, I would have named this PR:

"Backport PR #56114 on branch 2.1.x (CLN: Get rid of PyArray_GetCastFunc)"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

lithomas1 added a commit to lithomas1/pandas that referenced this pull request Dec 4, 2023
lithomas1 added a commit that referenced this pull request Dec 4, 2023
* Backport PR #56114: CLN: Get rid of PyArray_GetCastFunc

* fix bad indent?

* remove more unintended changes
@lithomas1 lithomas1 mentioned this pull request Dec 4, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compat pandas objects compatability with Numpy or Python functions Internals Related to non-user accessible pandas implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAINT: Remove use of PyArray_GetCastFunc
4 participants