use numpy sinc #3175

Merged
merged 2 commits into from Dec 29, 2013

Conversation

Projects
None yet
6 participants
@argriffing
Contributor

argriffing commented Dec 28, 2013

No description provided.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 28, 2013

Coverage Status

Coverage remained the same when pulling 7cc24c1 on argriffing:remove-sinc into 371b4ff on scipy:master.

Coverage Status

Coverage remained the same when pulling 7cc24c1 on argriffing:remove-sinc into 371b4ff on scipy:master.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 28, 2013

Member

+1, no need to duplicate this function.

I checked the numpy implementation, and that looks fine to me. The 1e-20 used there doesn't affect the precision it seems.

Member

rgommers commented Dec 28, 2013

+1, no need to duplicate this function.

I checked the numpy implementation, and that looks fine to me. The 1e-20 used there doesn't affect the precision it seems.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 28, 2013

Member

The tests for special.sinc should also be removed in this PR.

EDIT: maybe keep a very basic one to check that the function still exists in the special namespace.

Member

rgommers commented Dec 28, 2013

The tests for special.sinc should also be removed in this PR.

EDIT: maybe keep a very basic one to check that the function still exists in the special namespace.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 29, 2013

Coverage Status

Coverage remained the same when pulling 284023e on argriffing:remove-sinc into 371b4ff on scipy:master.

Coverage Status

Coverage remained the same when pulling 284023e on argriffing:remove-sinc into 371b4ff on scipy:master.

@argriffing

This comment has been minimized.

Show comment
Hide comment
@argriffing

argriffing Dec 29, 2013

Contributor

@rgommers I removed most of the testing.

Contributor

argriffing commented Dec 29, 2013

@rgommers I removed most of the testing.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 29, 2013

Member

Thanks, merging.

Member

rgommers commented Dec 29, 2013

Thanks, merging.

rgommers added a commit that referenced this pull request Dec 29, 2013

Merge pull request #3175 from argriffing/remove-sinc
Use numpy sinc instead of re-implementing it in scipy.special

@rgommers rgommers merged commit 7eca39e into scipy:master Dec 29, 2013

1 check passed

default The Travis CI build passed
Details

@rgommers rgommers referenced this pull request Dec 29, 2013

Closed

sinc #3069

@endolith

This comment has been minimized.

Show comment
Hide comment
@endolith

endolith Dec 29, 2013

Contributor

Would it make sense to move the tests and implementation into numpy instead of just deleting them? scipy's implementation makes more sense to me than something that depends on float inaccuracy.

Contributor

endolith commented Dec 29, 2013

Would it make sense to move the tests and implementation into numpy instead of just deleting them? scipy's implementation makes more sense to me than something that depends on float inaccuracy.

@argriffing

This comment has been minimized.

Show comment
Hide comment
@argriffing

argriffing Dec 29, 2013

Contributor

@endolith The numpy sinc passed the scipy tests, and numpy has its own more extensive sinc testing. On the other hand I'd like a new C sinc ufunc implementation in either numpy or scipy.

Contributor

argriffing commented Dec 29, 2013

@endolith The numpy sinc passed the scipy tests, and numpy has its own more extensive sinc testing. On the other hand I'd like a new C sinc ufunc implementation in either numpy or scipy.

@rgommers

This comment has been minimized.

Show comment
Hide comment
@rgommers

rgommers Dec 30, 2013

Member

New ufunc would indeed be nice. That should go into numpy and replace its current implementation imho.

Member

rgommers commented Dec 30, 2013

New ufunc would indeed be nice. That should go into numpy and replace its current implementation imho.

@argriffing

This comment has been minimized.

Show comment
Hide comment
@argriffing

argriffing Feb 23, 2016

Contributor

New ufunc would indeed be nice. That should go into numpy and replace its current implementation imho.

related: numpy/numpy#7322

Contributor

argriffing commented Feb 23, 2016

New ufunc would indeed be nice. That should go into numpy and replace its current implementation imho.

related: numpy/numpy#7322

@madphysicist

This comment has been minimized.

Show comment
Hide comment
@madphysicist

madphysicist Feb 24, 2016

Contributor

For the immediate future, numpy/numpy#7322 will actually remove the possibility of making np.sinc into a ufunc. However, I also have plans to work around that.

Contributor

madphysicist commented Feb 24, 2016

For the immediate future, numpy/numpy#7322 will actually remove the possibility of making np.sinc into a ufunc. However, I also have plans to work around that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment