Skip to content

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
@mdboom mdboom force-pushed the erfa/remove-cython branch from a7e11d1 to 05eba36 Compare March 2, 2015 22:14
@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.

3 participants