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

module 'twirl' has no attribute 'find_peaks' #19

Closed
mrobberto opened this issue May 31, 2023 · 4 comments
Closed

module 'twirl' has no attribute 'find_peaks' #19

mrobberto opened this issue May 31, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@mrobberto
Copy link

Bug Report

I am getting this error

module 'twirl' has no attribute 'find_peaks'

that is not present with an earlier version of the code.

How To Reproduce

pip install twirl
python
import twirl
twirl.[TAB]

Code sample

Environment

  • OS: macOS

Screenshots

Expected behavior

Additional context

@mrobberto mrobberto added the bug Something isn't working label May 31, 2023
@michealroberts
Copy link
Contributor

Hi @mrobberto, I hope you're well.

There have been some breaking changes in version v0.3.* which is consistent with a pre v1 from a semver sense, but we need to update the documentation it seems as that is referencing the old API.

I'll let @lgrcia advise on the equivalent function in v0.3.*.

@lgrcia
Copy link
Owner

lgrcia commented Jun 1, 2023

Hi @mrobberto, thanks for reporting this issue.

As @michealroberts explains, the function has been dropped from the last version. The main reason was that I found the function simple enough to remove it it from twirl and omit the photutils dependency. Despite the <v1, I reckon the versioning wasn't perfectly rigorous.

Looking back at the readme (that contains the only reference to this function), I think it is still nice to have such a function available. I propose to make a 0.3.3 patch that will include the following function:

# not tested - need docs
# -----------------------

from astropy.stats import sigma_clipped_stats
from photutils.segmentation import SourceFinder, SourceCatalog

def find_peaks(data, sigma=6):
  mean, median, std = sigma_clipped_stats(data, sigma=3.0) 
  segmented = SourceFinder(5)(data - median, sigma * std)
  cat = SourceCatalog(data, segmented)
  return (
      cat.to_table()
      .to_pandas()
      .sort_values("area", ascending=False)[["xcentroid", "ycentroid"]]
      .values
  )

and bring the photutils dependency back. Since it fixes a bug and brings back an omitted function, I think a patch is appropriate.

What do you think?

@mrobberto
Copy link
Author

hi Lionel,
thank you for your prompt feedback! I think reinstating the function would be very nice to keep code already developed, and in my experience find_peaks works pretty nicely. Note that is used in the notebooks/demos. This is typically what people like me uses to get quickly out of the woods...
Thank you again
Massimo

lgrcia added a commit that referenced this issue Jun 1, 2023
lgrcia added a commit that referenced this issue Jun 1, 2023
fix: find_peaks function back (#19)
@lgrcia
Copy link
Owner

lgrcia commented Jun 1, 2023

Thanks for the feedback. find_peaks is back in 0.3.3!

You mentioned notebooks/demos which seems to reference an outdated version of the repository. Is this something you found on an old clone or something that is still online?

The documentation should be hosted on twirl.readthedocs.io so I prefer to check we don't have alternative docs out there.

@lgrcia lgrcia closed this as completed Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants