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

Use Python/C API instead of Cython for ERFA wrappers #3521

Merged
merged 3 commits into from Mar 4, 2015

Conversation

mdboom
Copy link
Contributor

@mdboom mdboom commented Feb 19, 2015

This updates #3164 to use the new division of labor between Python and Cython, so the C side is even smaller now.

@embray embray added Affects-dev PRs and issues that do not impact an existing Astropy release erfa labels Feb 19, 2015
@embray
Copy link
Member

embray commented Feb 19, 2015

Looks good to me. I'm still hoping to complete #3181 at some point too once I have a chance to figure out a bytecode caching workaround. But I don't think it will conflict terribly with these changes.

@mhvk
Copy link
Contributor

mhvk commented Feb 19, 2015

Like the approach, but cannot really judge the implementation. Just to be sure something silly doesn't get passed by: numpy c code is full of Py_INCREF and Py_DECREF -- does the NPYITER stuff automatically take care of that for these purposes?

@mdboom
Copy link
Contributor Author

mdboom commented Feb 19, 2015

The only PyObject involved here is what's passed in (args) which is a borrowed reference (i.e. it doesn't need to be DECREF'd on exit. NpyIter is not a PyObject, so doesn't have a reference count.

@mdboom mdboom self-assigned this Feb 23, 2015
@mdboom mdboom added this to the v1.0.1 milestone Mar 2, 2015
@embray
Copy link
Member

embray commented Mar 4, 2015

I think this is pretty uncontroversial at this point. I see no reason not to merge.

embray added a commit that referenced this pull request Mar 4, 2015
Use Python/C API instead of Cython for ERFA wrappers
@embray embray merged commit 7345cf0 into astropy:master Mar 4, 2015
embray added a commit that referenced this pull request Mar 6, 2015
Use Python/C API instead of Cython for ERFA wrappers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Affects-dev PRs and issues that do not impact an existing Astropy release erfa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants