Allow regions to be passed to aperture_photometry

[havijw]

This allows regions.Region objects to be passed to aperture_photometry as long as they correspond to one of the Aperture subclasses, as in #1811.

My original approach was to create a wrapper Aperture class for regions that contained a center position and a Region, and allowed photometry with arbitrarily shaped regions. Per discussion below, I changed approaches to simply converting Region to Aperture without adding a new class.

To Do:

• [x] Incorporate regions into higher-level documentation
• [x] Add tests to make sure aperture photometry works correctly with regions (I have tested it in notebooks, but there are no tests to verify the integration)

[pep8speaks]

Hello @havijw! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2024-07-23 16:37:11 UTC

[larrybradley]

@havijw Thank you for taking the time to submit your pull request. I've reviewed your changes, and this is not the best solution. As mentioned in previous issues (e.g., #1811 and others), the idea here is to simply allow the aperture photometry functions to directly accept regions objects and then translate them (if possible) behind the scenes to aperture objects. We don't want to or need to add more classes to the existing API to support this functionality. All this would need is a simple check on the aperture/region input and if it's a region object, then try to translate it. The translation code already exists here (I didn't have a chance to implement it last Friday):

https://github.com/spacetelescope/jdaviz/blob/73e54d0e0f5516d2f4a066e86e88ebc3c1fb2a1c/jdaviz/core/region_translators.py#L136

Also, this would entail adding regions as an optional dependency. That means that any regions imports must occur within the function and not at the module level.

Would you be open to making these adjustments? If you need any help or further clarification, feel free to ask (but I will only be online intermittently until next week).

[havijw]

@larrybradley Thanks for the feedback! I see I was a bit overeager in implementing this; my intention was to allow any arbitrary region to be used for photometry, so a wrapper class made sense to me since regions (eg polygons) don't have a designated center position.

I'd be happy to make those changes, sounds like it should be straightforward. I assume the code from jdaviz should be copied, not imported?

Also, this would entail adding regions as an optional dependency. That means that any regions imports must occur within the function and not at the module level.

Should I still include it in the optional dependencies 'all' in pyproject.toml? I assume yes so that tox knows to install it, but just want to confirm.

[larrybradley]

I assume the code from jdaviz should be copied, not imported?

Yes. In jdaviz, we'll eventually remove that code and just call the translation function from photutils (it was actually the main motivation of #1811 to move that code upstream from jdaviz to photutils). That means the translation function must be public (not a private helper).

Should I still include it in the optional dependencies 'all' in pyproject.toml? I assume yes so that tox knows to install it, but just want to confirm.

Yes. All of those packages in all are optional dependencies.

Note you'll also need to add a skipif directive for any tests that require optional deps. Look in the aperture tests for other examples.

Thanks!