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

Complex-valued resampling #14

Closed
bmcfee opened this issue Apr 21, 2016 · 6 comments
Closed

Complex-valued resampling #14

bmcfee opened this issue Apr 21, 2016 · 6 comments
Assignees
Milestone

Comments

@bmcfee
Copy link
Owner

bmcfee commented Apr 21, 2016

Cython's fused types should make this trivial, but for whatever reason, it didn't work out of the box.

We may have to handle complex-valued data explicitly.

@bmcfee bmcfee self-assigned this Apr 21, 2016
@bmcfee bmcfee added this to the 0.2.0 milestone Apr 21, 2016
@larsoner
Copy link

FYI the fused types with complex values gets a bit wonky. You can read a long discussion (struggle) I recently had with it here or better, just look at what the solution ended up being. Maybe there is something helpful in there for you.

@larsoner
Copy link

...now it's coming back to me a bit -- the workaround was to make it so that whatever fused type DTYPE_t I used as an argument was the only fused type in the signature, but it was fine to use it for multiple arguments. When I used more than one fused type in the signature (so that e.g. the kernel argument and the data argument could be different types), it tried to make all versions of the function, some of which had incompatible types when used within the function. Cython provides no way to limit the set of argument combinations to generate functions for, so it seemed to be the only way out.

@bmcfee
Copy link
Owner Author

bmcfee commented Jul 15, 2016

Aha, I kinda suspected it night be something like that. Thanks for confirming!

It does seem like the kind of thing that ought to be fixed in a future cython release though, so maybe we can just wait it out?

@larsoner
Copy link

It does seem like the kind of thing that ought to be fixed in a future cython release though, so maybe we can just wait it out?

I thought I opened an issue for it somewhere, but looking back I don't have any record of it. I should probably do that...

In any case, even once the request is opened, there's no telling how long it would take them to implement it. Maybe it's something that wouldn't be so hard to contribute ourselves. I'll try to find time to look into it.

In the case of my code, the extra overhead of casting both to the biggest type was assumed to be small, so it didn't seem like too bad a compromise (a bit of extra casting and mem use) to gain proper support for complex types. So my feeling is that it would depends on how much you'd have to compromise your code to implement the workaround.

@bmcfee
Copy link
Owner Author

bmcfee commented Jul 15, 2016

Sure.. alternately, it wouldn't be terrible to just resample the imaginary and real components independently and then recombine them.

So my feeling is that it would depends on how much you'd have to compromise your code to implement the workaround.

That would be easy, but I think it would invoke another data copy, so i'd like to avoid it if possible.

bmcfee added a commit that referenced this issue Sep 8, 2017
@bmcfee
Copy link
Owner Author

bmcfee commented Sep 8, 2017

FWIW: #57 gives us complex-valued resampling for free, so we can close this out after merging.

@bmcfee bmcfee closed this as completed in bf57516 Sep 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants