Switch from PeakUtils to Scipy.Signal

[TheSkyentist]

Fix addressing #235.

[gbrammer]

Thanks, looks pretty straightforward. It looks like you figured out the mapping between the two modules with the thresholding. Rather than overloading the height= keyword with some array transformations, could you consider writing a function (and a test) like utils.threshold_peak_indexes or something that takes "threshold" and "min_separation" keywords and behaves like peakutils.indexes?

[TheSkyentist]

I'd be willing to do this, but perhaps this would be quite a short function in utils. Perhaps I can clean up the creation of the height value to make it clearer. Let me make some changes and let me know if they make things more readable or if you'd prefer to make a new function.

[gbrammer]

There's nothing wrong with a short function! Just to make the behavior explicit and easily testable.

[TheSkyentist]

Okay I'll refactor in a bit. Also a note that the CI is failing because the latest photutils fails with the latest drizzlepac (as of 3 hours ago). Drizzlepac has changes in place for their next release, but for now we might want to pin photutils as well.

On Fri, Jun 28, 2024 at 11:11 PM Gabe Brammer @.***> wrote:

There's nothing wrong with a short function! Just to make the behavior explicit and easily testable.

— Reply to this email directly, view it on GitHub https://github.com/gbrammer/grizli/pull/236#issuecomment-2197666115, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEB6FNHEUIGSKYVJUHQRK2TZJXGPBAVCNFSM6AAAAABKCHCFLKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJXGY3DMMJRGU . You are receiving this because you authored the thread.Message ID: @.***>

[gbrammer]

OK, this looks great. Can you add a test to grizli/tests/test_utils.py for the new function? The test doesn't have to be super comprehensive, just something that verifies that the function works as expected given numpy, scipy, etc.

[TheSkyentist]

Good call, I've added the relative tests.