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

Remove unused extract() method #21

Open
svank opened this issue Apr 6, 2023 · 1 comment
Open

Remove unused extract() method #21

svank opened this issue Apr 6, 2023 · 1 comment
Assignees

Comments

@svank
Copy link
Contributor

svank commented Apr 6, 2023

PatchCollectionABC (and also CoordinatePatchCollection) has an .extract() method that doesn't appear to be used---the contents of extract() are more-or-less reproduced inside find_stars_and_average. I'm betting either find_stars_and_average should use extract, or extract should be removed as un-used.

If you keep .extract(), the test test_coordinate_patch_collection_extraction_many_coordinates in test_fitter.py generates warnings (from inside PatchCollectionABC.add()) because duplicate coordinates are generated for the test case. Those can be silenced by de-duplicating the generated coordinates by adding coords = list(set(coords)) at the start of the test function (or maybe there's a cleaner way by adjusting how hypothesis generates the coordinates).

@jmbhughes
Copy link
Member

I believe extract is a vestigial function that lingers after the more helpful find_stars_and_average was developed. Originally, I was extracting patches based on a stellar catalog instead of SEP. It might be nice for find_stars_and_average to use extract. I've been hoping to decrease the complexity of find_stars_and_average anyway; it's on the long side of functions at 100+ lines. Maybe we can modularize its functionality some by using extract?

Anyway, thanks for raising this. I can look into it after the PUNCH review next week.

@jmbhughes jmbhughes self-assigned this Jun 7, 2023
@jmbhughes jmbhughes changed the title Unused extract() method Remove unused extract() method Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants