ENH: Added optional scale parameter to sinc to enable non-normali…
#7322
Conversation
numpy/lib/function_base.py
Outdated
| if scale == 1.0: | ||
| y = where(x == 0, 1.0e-20, x) | ||
| else: | ||
| y = scale * where(x == 0, 1.0e-20, x) |
There was a problem hiding this comment.
To reduce code duplication, might be better to write:
y = where(x == 0, 1.0e-20, x)
if scale != 1.0:
y *= scale
|
Code looks fine to me modulo small comments noted above. This is a public API change, so can you send a note to the list soliciting feedback? (I can read the code, but I've never used |
bbb0a6b to
989c3ca
Compare
|
I made the changes you requested, and some extra. I think broadcasting is fine (certainly harmless if done correctly), so I updated the docs accordingly and added a test. This PR came from the fact that I started using |
989c3ca to
0ac37c4
Compare
0ac37c4 to
42a8665
Compare
|
I like how the PR cleans up |
|
@mhvk. I was trying to avoid A) creating as many temp arrays as possible, B) Doing a multiply-then-divide by As far as the |
|
The easiest thing to do is probably to make a I'm not all that convinced that there is some huge need for setting |
42a8665 to
333e6cc
Compare
|
@ewmoore I am very hesitant to make |
|
Sure. Extending ufuncs to take keyword args is also a large, backward incompable project since there is no mechanism for passing them to the inner loop function right now. That |
|
@ewmoore I am not sure why the project is backward incompatible. Previous ufuncs would implicitly have no keyword arguments defined. It would not break anything if done carefully. Also, it looks like Python 2 is unhappy with keyword-only arguments. I will revert my last change but still note in the docs that support for |
333e6cc to
4a7a56a
Compare
|
@madphysicist: adding kwargs to ufuncs will require rewriting big chunks of how ufuncs work -- it's very very not trivial. |
|
@njsmith. I am aware of that. In fact, that is part of my motivation for the undertaking. It will probably take a while, but I suspect that I will know a lot more about numpy internals if I succeed. |
|
Btw, my initial idea was to just add a hook function to the ufunc structure for processing the kwargs. The hook would be NULL by default. It would return a void pointer to some structure that the loop would know how to use and an optional deallocator for the struct. The struct only gets passed to the loop if the hook is non-NULL, so backwards compatibility will not be broken (much) since existing loops would not need a rewrite. The deallocator gets applied at the end if it is non-NULL. Let me know if this sounds reasonable or even sane. I still don't know enough to have worked out any of the major details. |
|
@madphysicist: you will have to break ABI to make this happen. Which is okay -- we desperately need to break the ufunc ABI for a number of reasons :-). But if you're interested in working on this -- which is awesome -- then the first place to start is to read this thread and it would probably be good to schedule a video conference or something to go over how this fits into the bigger plan? (Nothing discussed in that thread has actually been implemented yet, because I've been too busy with other stuff, so you can see how I'd be eager to help someone else get started ;-)) |
|
@njsmith I am very interested in working on this after looking at the thread, your NEP and the related reports you made. However, I feel the need to point out (again) that I am currently very new to the C internals. I am in the process of reading the docs at http://docs.scipy.org/doc/numpy/reference/c-api.html and following along in the code. While I am beginning to grasp the overall concept, and even form some ideas on how to approach the subject, it will take some time before I can get up to speed to the point where I can help with meaningful design decisions. If you are OK with that, I would be glad to make time for a video conference or something equivalent in the next couple of weeks to discuss the path going forward. |
|
@njsmith Is there a copy of the NEP draft somewhere outside the thread? Also, would you mind if I joined the thread? I have an idea that I would like to throw out there. |
|
☔ The latest upstream changes (presumably #7373) made this pull request unmergeable. Please resolve the merge conflicts. |
e5d4d21 to
55bd798
Compare
|
Squawk |
|
☔ The latest upstream changes (presumably #7198) made this pull request unmergeable. Please resolve the merge conflicts. |
55bd798 to
d44c7fe
Compare
|
@madphysicist you should definitely speak up on the numpy-discussions list. You might start another thread if that one is very old, but we are always happy to hear from new contributors with ideas! |
|
@madphysicist: Sorry I missed replying to your comments above earlier. Yeah, like @shoyer says, please do speak up on the mailing list about your ideas :-). And for setting up a video call, if you're still interested send me an email @ njs@pobox.com with some possible times and we can figure it out there? I'm on California time, and usually free in the afternoons -- but no need to clutter up the bug tracker with scheduling details :-). |
|
Is it worth keeping this PR around given the upcoming ufunc changes? Personally I would lean towards yes since A) the changes might take a while, and B) I will do my best to ensure that the extra parameter is retained when sinc becomes a ufunc of the new variety. |
|
☔ The latest upstream changes (presumably #7518) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This is just my opinion, but I think it's overkill to allow a general scaling factor. An alternative could be to have separate trig functions analogous to |
|
AFAIK, one reason for cos_pi, sin_pi is that sometimes you need to get the zeros exactly right. Not sure if there's an use case for sinc where this is important, but the current implementation doesn't guarantee it either. |
|
@argriffing. I think your solution may be the right one. If I can not figure out a way to guarantee the zeros for arbitrary scaling, I will go with your suggestion. In the meantime, this PR can be considered to be on hold. |
|
☔ The latest upstream changes (presumably #7704) made this pull request unmergeable. Please resolve the merge conflicts. |
|
☔ The latest upstream changes (presumably eb12b71) made this pull request unmergeable. Please resolve the merge conflicts. |
| elif scale == 'unnormalized': | ||
| scale = 1.0 | ||
| else: | ||
| raise ValueError("{} is not a valid scaling for `sinc`".format(scale)) |
There was a problem hiding this comment.
This seems like overkill - I don't think passing strings helps at all here.
| if isinstance(x.dtype, np.inexact): | ||
| min_val = np.finfo(x.dtype).tiny | ||
| else: | ||
| min_val = np.finfo(np.float_).tiny |
There was a problem hiding this comment.
Can we try and at least get this bit in?
|
|
||
| y = where(x == 0, min_val, x) | ||
| if np.any(scale != 1.0): | ||
| y *= scale |
There was a problem hiding this comment.
Is doing the check here really buying anything?
|
|
||
| The sinc function is :math:`\\sin(\\pi x)/(\\pi x)`. | ||
| For arbitrary scales, the function is defined as | ||
| :math:`\frac{\sin(scale \cdot x)}{scale \cdot x}`. In this case, the |
There was a problem hiding this comment.
\cdot is misleading, as it implies np.dot()
|
@eric-wieser. This PR has been on hold for a while, and I think I may end up closing it and starting anew (but not sure yet). The correct implementation would be to make two functions, |
…zed function. Currently, `sinc(x)` is defined as `sin(pi*x)/(pi*x)`. It is sometimes convenient to have the mathematical definition available: `sin(x)/(x). In fact, any scaling is now possible with a scalar input. Tests included. Also modified the docs for `where` very slightly.
|
@madphysicist I am closing this, since I guess this lost traction and it sounds like we probably need some decision on the best way forward (you mentioned that this PR is currently not quite the right way). We can reopen this at any point. I have opened the issue gh-13457 about this so your start does not get lost in either case! |
|
@seberg. Under the circumstances, I think that's the right thing to do. I frankly don't even remember what I was doing back then to motivate the PR. |
…zed function.
Currently,
sinc(x)is defined assin(pi*x)/(pi*x). It is sometimes convenient to have the mathematical definition available: `sin(x)/(x). In fact, any scaling is now possible with a scalar input. Tests included.Also modified the docs for
wherevery slightly.