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

ENH: expose datetime64 conversion functions #16364

Closed
wants to merge 2 commits into from

Conversation

jbrockmendel
Copy link
Contributor

These would allow pandas to remove our copy/pasted implementations.

cc @WillAyd @mroeschke anything else we might need?

@@ -287,8 +286,7 @@ convert_timedelta_to_pyobject(npy_timedelta td, PyArray_DatetimeMetaData *meta);
/*
* Converts a datetime based on the given metadata into a datetimestruct
*/
NPY_NO_EXPORT int
convert_datetime_to_datetimestruct(PyArray_DatetimeMetaData *meta,
int convert_datetime_to_datetimestruct(PyArray_DatetimeMetaData *meta,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is sufficient, extension modules aren't allowed to link against each other. This needs to go through the function table like the other NUMPY_API functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, ill see if i can track that down. Before I do, is this likely to be accepted once working?

Copy link
Member

Choose a reason for hiding this comment

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

I think we'd need to think about the function names, but in principle it seems sensible to me to expose the npy_datetimestruct type and some APIs that consume it.

@WillAyd
Copy link
Contributor

WillAyd commented May 27, 2020

For completeness we might want to upstream the timedelta changes as well:

https://github.com/pandas-dev/pandas/blob/707640cc89dd16bff00b62950b3fa27f3b642d5f/pandas/_libs/tslibs/src/datetime/np_datetime_strings.h#L90

Probably in a separate PR, maybe a pre-cursor. The above is used in JSON so I think we wouldn't be able to completely simplify our dependency structure without tacking that as well

@bashtage
Copy link
Contributor

@eric-wieser Not sure why this is tagged random.

@jbrockmendel
Copy link
Contributor Author

Not sure why this is tagged random

@bashtage best guess is that on a previous PR implementing test_cython we adapted the setup from random.tests.test_extending into a fixture and there was an offhand mention of using that pattern in the random tests.

Base automatically changed from master to main March 4, 2021 02:04
@jbrockmendel
Copy link
Contributor Author

Closing in favor of #21199.

jbrockmendel added a commit to jbrockmendel/numpy that referenced this pull request Mar 23, 2022
jbrockmendel added a commit to jbrockmendel/numpy that referenced this pull request Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants