photutils icon indicating copy to clipboard operation
photutils copied to clipboard

Deprecating Apertures

Open sushobhana opened this issue 7 years ago • 5 comments

Since the Regions is getting matured , we can talk about possibilities of how we can replace the Aperture s with it. I think they share a lot in common and would be easy to integrate. I would like to know which would be the best place to start and what are the possible difficulties which we may be facing?

sushobhana avatar Jun 28 '18 14:06 sushobhana

I have gathered some comments in this doc https://docs.google.com/document/d/1v72eCOo2qb8bcHEmPUBY3YxIyUuOTKUvTIujAdfHgHI/edit?usp=sharing

sushobhana avatar Jun 28 '18 14:06 sushobhana

Thanks, @sushobhana. Yes, the plan has been to migrate the general features of aperture-photometry functionality from photutils to regions (and then regions to astropy core). This has been on my to-do list, but hasn't been high priority (I've also been hoping regions would go into astropy sooner than later). Thanks for your work on improving regions!

I think all of the general functionality of aperture-photometry has already been moved to regions, including the low-level Cython geometry functions and the Mask and BoundingBox classes (although with some small differences). However, I am strongly not in favor of deprecating the photutils Aperture classes to be replaced by the regions PixelRegion/SkyRegions classes or changing the aperture_photometry function. Instead, I was envisioning that the regions classes would be used as Mixin classes in the Aperture classes to replace the methods in common (i.e. the underlying geometry/Mask machinery), but keeping the Aperture-specific methods. One huge difference between them (beside API differences) is the Aperture classes are usually used to define multiple apertures, all with the same shape but different positions, within one object (i.e. positions is an array of positions). Removing that functionality doesn't make any sense to me. IIRC, it was decided long ago that the regions objects would be singular (one shape, one position).

Some other quick thoughts:

  • Changing to the regions geometry classes is a trivial change of the imports (they are identical copies)

  • The ApertureMask class is called Mask in regions. IMHO, Mask seems too general a name (perhaps RegionMask is better?)

  • The BoundingBox class in photutils has a very useful plot convenience method that I would very much like to keep (for interactive work) that is currently not in regions.

CC: @astrofrog, @keflavich

larrybradley avatar Jul 02 '18 20:07 larrybradley

I agree that we shouldn't deprecate the aperture classes, just rework them so that behind the scenes they use classes from regions. I think photutils has enough users that we wouldn't want to break something so fundamental.

astrofrog avatar Jul 03 '18 22:07 astrofrog

I understand how breaking things will be difficult for users. Also, the major problem would be handling multiple apertures as @larrybradley says. So, the plan would be to remove the overlapping classes such as masks, bounding box and geometry functions once regions matures?

sushobhana avatar Jul 04 '18 15:07 sushobhana

It might be worth at least just accepting regions for now in addition to apertures and converting to apertures behind the scenes?

astrofrog avatar Oct 13 '21 23:10 astrofrog