Closed shivangeerathi closed 5 years ago
Hi, As a test, I tried this example, with small data values (~1e-11) and I get the same error.
@shivangeerathi Actually @ibusko wrote that notebook and most of the isophote-fitting code. He is the original author of the IRAF ellipse task.
@ibusko Can you take a look?
@shivangeerathi , @larrybradley yes, sure. I will take a look and report back here.
@shivangeerathi after some investigation, I found that the cause of this behavior is that the radial gradient computation seems to lose precision when the pixel values are too small, as in the case of your image. The algorithm has a stopping criterion that is triggered by the relative error in the gradient. When this error is larger than a pre-set amount, it stops to iterate. In the case of images with low values in the pixels, it seems to trigger the criterion at the very first attempt to fit the initial ellipse.
I am still looking in more depth for the exact cause, but in the meantime, I have a question: did you try to multiply your image by a large constant before running the algorithm on it? In case you do that and it works, or at least passes the first ellipse, it would tell us that it's definitely the gradient computation, and not something else.
@ibusko Yes, I have tried that. The algorithm seemed to work fine when I multiplied the image with 1e12 (thus making pixel values lie between -2 and 618). But multiplying with smaller values (such as 1e10, 1e11) gives an empty list of isophotes.
Also, I do not know if it is useful, but I get decent fit when using 'ellipse' task in IRAF on the original image with extremely small (~1e-12) pixel values.
@shivangeerathi @larrybradley the ultimate cause of the gradient error problem is that the code has a hard-coded hidden assumption about the order of magnitude of the data values stored in the input image array. With low values such as in this case, that assumption causes the calculation to be skipped, with the result that the algorithm acts as if a fit is not possible.
I removed the assumption about the data values, and that seems to have fixed the problem.
However, running the tests and the notebooks, I found that the modified calculation sometimes creates slightly different results when compared with the code with the hard-coded assumption. Overall the new results are either identical, or close to the old ones, but there are cases with outliers. In particular, the new fixed code seems to be more stable in the low SNR regions (outskirts of galaxies), with the end result that the fit proceeds to larger values of semi-major axis, when with the old code it would have stopped due to inability to compute the radial gradient.
Now I have a question about backwards compatibility. Should we deploy the solution as is, resulting in possible backwards-compatibility and reproducibility problems to some users, or should we add a user-settable flag that can be used by the user to force the use of one of the two possible computations? And if so, what should be the default for that flag? Do by default what the algorithm has been doing so far, but in the process losing the better stability of the new code? Or do by default the new code, resulting in a possible surprise to the end user?
Thanks for the update!
Since the results from the fixed version of the code differ only slightly from the previous version of the code, I think it makes more sense to deploy the latest version of the code. Plus, it will be good for the future development of the code.
I think this is a clear-cut case when breaking backwards compatibility is justified. The change log can articulate the origin of any minor differences and anyone wanting the previous version of the code (albeit without all the latest improvements) can simply use an older tag.
On Sun, Aug 11, 2019, 11:05 AM shivangeerathi notifications@github.com wrote:
Thanks for the update! Since the results from the fixed version of the code differ only slightly from the previous version of the code, I think it makes more sense to deploy the latest version of the code. Plus, it will be good for the future development of the code.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/astropy/photutils/issues/920?email_source=notifications&email_token=AAK5SDHEZXQTAZBJYQKEA23QEATCPA5CNFSM4IKKIDGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4BCSEQ#issuecomment-520235282, or mute the thread https://github.com/notifications/unsubscribe-auth/AAK5SDHZDQRRPPRUE2QUITDQEATCPANCNFSM4IKKIDGA .
@ibusko Arguably this is a bug fix, so in this case I think breaking backwards compatibility is fine. Please be sure to add a change log entry.
Fixed in #934.
I ran on this warning too. I am fitting on a HSC image:
@Firestar-Reimu This particular issue was fixed in #934 and was released several years ago in photutils v0.7
. If you are still having trouble, please open a new issue.
Hello, I am new to photutils. I am trying to measure surface brightness profile of a dwarf elliptical galaxy (see image below). Using ellipse.fit_image(), I either get an empty isophote list (using default fit_image() parameters) or the following error (using parameters used in @larrybradley 's notebook 4):
WARNING: No meaningful fit was possible. [photutils.isophote.ellipse]
The pixel values are in arbitrary units and have values around ~1e-12. To see if this was causing the trouble, I multiplied pixel values with 1e12 and performed the fitting again. It worked! Of course, this is a very random quick fix. So my question is, does photutils have issue with crunching extremely small numbers (like, for example, 1e-12) ? Or am I missing something ? Please help!
On a different note, I was wondering if there already exists some built-in feature in photutils to provide a data quality file (like a mask file in IRAF).
Thank you in advance!