regularizepsf icon indicating copy to clipboard operation
regularizepsf copied to clipboard

Remove unused extract() method

Open svank opened this issue 2 years ago • 1 comments

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).

svank avatar Apr 06 '23 21:04 svank

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 avatar Apr 06 '23 23:04 jmbhughes