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

Add implementation using Numpy C UFunc API #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mdboom
Copy link

@mdboom mdboom commented Jul 17, 2014

For now, the tests from #1 can be used with this extension module -- but I think eventually we want to centralize the test suite and have it run against all of the implementations as a fixture.

Just to compare and contrast with #1: By using the UFunc API, this lets Numpy handle the broadcasting for us upfront, rather than handling it ourselves. I think the per-erfa-function lines-of-code is probably a little lower for this approach as well (though there is some extra helper functions at the top, we only have to get that right once).

@astrofrog
Copy link
Contributor

Nice! I agree that we should centralize the test suite and also probably do some speed benchmarks.

'c_ufunc',
['c_ufunc.c'],
libraries=['erfa']
)],
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I think this is missing the numpy include path?

@astrofrog
Copy link
Contributor

@mdboom - could you show a usage example? At the moment:

In [21]: eraD2dtf('UTC',2,3.,4.)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-21-fc54acac0e60> in <module>()
----> 1 eraD2dtf('UTC',2,3.,4.)

TypeError: Not implemented for this type

Am I using it wrong?

@mdboom
Copy link
Author

mdboom commented Jul 18, 2014

@astrofrog: I see that too. I did this kind of hastily, and I think I may have messed something up... Stay tuned...

@mdboom
Copy link
Author

mdboom commented Jul 18, 2014

I'm running into the same problem here -- there doesn't seem to be a way to have ufuncs over string fields. Looking at the code, it seems there used to be a way but it got sort of papered over over time 😦

As discussed in #1, though, that parameter doesn't need to be vectorized, so we can wrap this in another function that just takes it as a scalar. That's where things start to get a little hairy, of course.

@mdboom
Copy link
Author

mdboom commented Jul 18, 2014

I think #1 is already to be considered generally superior to this. It seems the UFunc API just isn't flexible enough for our needs -- I was hoping it would be a win since it handles the broadcasting etc. for us.

It may make sense to do a C implementation of #1 for comparison at some point, but I think I've convinced myself that the UFunc API is insufficient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants