Repository navigation
Moffat1D and Moffat2D derivatives #8094
Description
The derivatives in Moffat1D are wrong:
astropy/astropy/modeling/functional_models.py
Line 2223 in 2b8fe13
They should be replaced by:
@staticmethod
def fit_deriv(x, amplitude, x_0, gamma, alpha):
"""One dimensional Moffat model derivative with respect to parameters"""
fac = (1 + (x - x_0) ** 2 / gamma ** 2)
d_A = fac ** (-alpha)
d_x_0 = (2 * amplitude * alpha * (x - x_0) * d_A / (fac * gamma ** 2))
d_gamma = (2 * amplitude * alpha * (x - x_0) ** 2 * d_A /
(fac * gamma ** 3))
d_alpha = -amplitude * d_A * np.log(fac)
return [d_A, d_x_0, d_gamma, d_alpha]
In Moffat2D:
astropy/astropy/modeling/functional_models.py
Line 2301 in 2b8fe13
only derivative wrt gamma is wrong. It should be
d_gamma = 2 * amplitude * alpha * d_A * rr_gg / ((1 + rr_gg) * gamma ** 3)
Activity
pllim commentedon Nov 9, 2018
These were originally added by @adonath as part of #1251 and has since been renamed a few times. It was
Beta...in that PR.adonath commentedon Nov 9, 2018
I remember that for the
Beta1DandBeta2Dmodels I computed the derivatives and reference values in a notebook using using Sympy (http://nbviewer.jupyter.org/gist/adonath/6088712). So I'm pretty sure it used to be correct at that time. Maybe when the rename toMoffat1DandMoffat2Dhappened, the parametrization changed as well and the derivatives where not adapted? From a quick look in the code it seems there are no unit tests for the model derivatives.@ycopin I didn't check whether your proposed formulae are correct, please check against Sympy or some other algebra library and post the result as a reference. In case you still find the current formulae are incorrect, would you mind preparing a minimal pull request with the correct ones? Ideally one should add unit tests for the model derivatives, but this is a somewhat larger project...
ycopin commentedon Nov 9, 2018
I used Wolfram Alpha for the derivatives, but I trust the derivatives in the aforementioned notebook are correct. The issue is that they were not properly implemented in the code (in particular the
d_A ** alphaterms, where it should just bed_A).For sure, the derivatives are currently incorrect, and the fit does not converge if not using ad-hoc numerical derivative estimates. New derivatives solve the issue (at least in my 1D case).
I'll try to prepare a PR, but that might take more time...
Fix ``Moffat1D`` and ``Moffat2D`` derivatives. [astropy#8094]
ycopin commentedon Nov 10, 2018
BTW, I wonder why this was not detected before, as it seems there are some derivative tests in modeling.
Merge pull request #8108 from ycopin/Issue8094
Merge pull request #8108 from ycopin/Issue8094
Merge pull request astropy#8108 from ycopin/Issue8094
Merge pull request astropy#8108 from ycopin/Issue8094
chris-simpson commentedon Sep 29, 2020
You got this one wrong. The derivative in Moffat2D was correct and the PR made it wrong. There's only a single power of gamma in the denominator, not 3, because 2 of them have been subsumed into
rr_gg. I'll use Wolfram alpha for the calculus so you don't have to check mine...So there's only a single isolated power of gamma remaining in the denominator.
saimn commentedon Sep 29, 2020
Finding the same issue with
d_gammawith Sympy:Full notebook here: https://gist.github.com/saimn/f79197fe45bb96d90f6ca3fa54910ac2
The other fixes (implemented in #8108) for Moffat1D are correct.
ycopin commentedon Sep 30, 2020
I'm sorry I introduced a bug in Moffat2D derivatives... :-/ As mentioned earlier, verifying the formal derivatives using e.g. Wolfram is not necessarily sufficient, but one should check the actual implementation, thanks @saimn for doing so (even if the tested implementation might not be the most efficient). I still wonder why this implementation error was not caught by a test comparing formal and numerical derivatives...