You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
This repository was archived by the owner on Dec 15, 2020. It is now read-only.
Repository navigation
This repository was archived by the owner on Dec 15, 2020. It is now read-only.
When adding a small 2d array to a larger one, the behaviour of add_array_2d seems ill-defined when the shapes do not align nicely. At the moment this crashes:
I'd like to add that I decided to review the behaviour of add_array_2d because I am experiencing the following problem with photutils.subtract_psf, which is related:
Maybe the issue is that imageutils.add_array_2d uses the center as position ... probably it's better to use the edges instead (either via integers or slice objects), like scipy.ndimage and scikit-image do. I tried to think about a good API for image utils that operate on parts of images only at some points and started collecting some thoughts here but didn't get to a conclusion or good implementation ... now is the time to do better for astropy.image!
@barentsen Thank you for revealing this issue. I had quick look at the code and the problem is rather related to the fact that smaller array has an even shape. Originally this code was meant to be only used with odd shaped PSFs/PRFs, but the issue with subtract_psf shows, that there is a need to also handle even shaped cases properly, which can occur close to data boundaries, when only a fraction of the PSF/PRF overlaps with the data.
I agree with @cdeil to think about a general solution for handling image slices, bounding boxes, cutouts etc., because it is needed at so many places (only in photutils there are three different kind of implementations of similar functionality...).
I now remember where we discussed this before ... in #4 I proposed to add a BoundingBox class.
I still think it would be a good idea to centralise the image slicing code (generating min / max from center and extent; handling edges and overlaps, ...) in a class. I'm not sure it should hold references to the large image array or the cutout image array ... I think only trying to rewrite the existing code that deals with slices in photutils, gammapy and ccdproc would reveal what is best to implement and for the end user.
Should the generic problem of "adding arrays" perhaps be treated as a reprojection problem, rather than a slicing problem? At least for the purpose of photutils.subtract_psf, I would expect add_array_2d to behave as follows:
@barentsen The handling of subpixel positions, that you just described, is done internally in the photutils.DiscretePRF class, so there was no need for this functionality in subtract_psf and add_array_2d. Generally speaking you are totally right, that's the kind of behaviour you would expect for an add_array_2d function .
The original bug here is fixed by #42. @cdeil - I agree we may still want to find a better API, so feel free to open an issue for that if you have ideas.
Activity
barentsen commentedon Nov 28, 2014
I'd like to add that I decided to review the behaviour of
add_array_2dbecause I am experiencing the following problem withphotutils.subtract_psf, which is related:cdeil commentedon Dec 2, 2014
@adonath I think you wrote this ... any thoughts?
Maybe the issue is that imageutils.add_array_2d uses the center as
position... probably it's better to use the edges instead (either via integers or slice objects), likescipy.ndimageandscikit-imagedo. I tried to think about a good API for image utils that operate on parts of images only at some points and started collecting some thoughts here but didn't get to a conclusion or good implementation ... now is the time to do better forastropy.image!adonath commentedon Dec 2, 2014
@barentsen Thank you for revealing this issue. I had quick look at the code and the problem is rather related to the fact that smaller array has an even shape. Originally this code was meant to be only used with odd shaped PSFs/PRFs, but the issue with
subtract_psfshows, that there is a need to also handle even shaped cases properly, which can occur close to data boundaries, when only a fraction of the PSF/PRF overlaps with the data.I agree with @cdeil to think about a general solution for handling image slices, bounding boxes, cutouts etc., because it is needed at so many places (only in photutils there are three different kind of implementations of similar functionality...).
Fix for astropy#40 and astropy#29
cdeil commentedon Dec 2, 2014
I now remember where we discussed this before ... in #4 I proposed to add a
BoundingBoxclass.I still think it would be a good idea to centralise the image slicing code (generating min / max from center and extent; handling edges and overlaps, ...) in a class. I'm not sure it should hold references to the large image array or the cutout image array ... I think only trying to rewrite the existing code that deals with slices in
photutils,gammapyandccdprocwould reveal what is best to implement and for the end user.Fix for astropy#40 and astropy#29
barentsen commentedon Dec 2, 2014
Should the generic problem of "adding arrays" perhaps be treated as a reprojection problem, rather than a slicing problem? At least for the purpose of
photutils.subtract_psf, I would expectadd_array_2dto behave as follows:... whereas right now the output is:
adonath commentedon Dec 2, 2014
@barentsen The handling of subpixel positions, that you just described, is done internally in the
photutils.DiscretePRFclass, so there was no need for this functionality insubtract_psfandadd_array_2d. Generally speaking you are totally right, that's the kind of behaviour you would expect for anadd_array_2dfunction .I have a quick fix for the issue with
subtract_psfin https://github.com/adonath/imageutils/tree/fix_for_%2340, which should resolve the issue with even shaped PSFs/PRFs close to the data boundaries.Fix for astropy#40 and astropy#29
Merge pull request #42 from adonath/fix_for_#40
astrofrog commentedon Dec 14, 2014
The original bug here is fixed by #42. @cdeil - I agree we may still want to find a better API, so feel free to open an issue for that if you have ideas.
Fix for astropy/imageutils#40 and astropy/imageutils#29
Fix for astropy/imageutils#40 and astropy/imageutils#29
Fix for astropy/imageutils#40 and astropy/imageutils#29
astrofrog commentedon Jan 11, 2016
This is now fixed in the Astropy version. @cdeil - feel free to open an issue on Astropy if you think the BoundingBox approach should be implemented.