Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix: propagation of measurement uncertainties into parameter covariances #14519

Merged
merged 3 commits into from Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 27 additions & 9 deletions astropy/modeling/fitting.py
Expand Up @@ -1240,15 +1240,26 @@
for _ in weights * np.array(model.fit_deriv(x, y, *params))
]

def _compute_param_cov(self, model, y, init_values, cov_x, fitparams, farg):
def _compute_param_cov(
self, model, y, init_values, cov_x, fitparams, farg, weights=None
):
# now try to compute the true covariance matrix
if (len(y) > len(init_values)) and cov_x is not None:
sum_sqrs = np.sum(self.objective_function(fitparams, *farg) ** 2)
dof = len(y) - len(init_values)
self.fit_info["param_cov"] = cov_x * sum_sqrs / dof
self.fit_info["param_cov"] = cov_x
if weights is None:

Check warning on line 1249 in astropy/modeling/fitting.py

View check run for this annotation

Codecov / codecov/patch

astropy/modeling/fitting.py#L1248-L1249

Added lines #L1248 - L1249 were not covered by tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also give the option to toggle the behavior like the absolute_sigma option that scipy.optimize.curve_fit gives users for the case when no weights are passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand giving people options is generally a good thing, and the generic name of the kwarg is weights. But we say in the docstring that weights = 1 / sigma, which is not true if absolute_sigma == False.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, so then the question becomes "should we add an option to disable this change". That is add an option which allows users to return the behavior of the weights to how they previously worked? My concern here lies with the fact that this change will technically change this computation's output in some cases. This can have knock-on effects for downstream users, who may wish to return to the previous behavior.

@pllim what is your opinion here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is add an option which allows users to return the behavior of the weights to how they previously worked?

Unless the old way is completely wrong, it is generally good to provide backward compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue the old way is completely wrong. We offered a keyword to adjust the weights, but did not compute the result based on the weights.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also – the user can have "perfect" backwards compatibility if they set weights = None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I told Brett offline, API change is acceptable as long as it is not backported. And if an extra option lets people get the old results, then fine by me but ultimately it is up to @astropy/modeling .

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle, I am okay with this change so long as it is not backported. However, we should probably advertise the change with a "whats-new". So that it is clear when reading the changes for astropy 5.3 that this behavior will be slightly different.

# if there are no measurement uncertainties given in `weights`,
# fall back on the default behavior in scipy.optimize.curve_fit
# when `absolute_sigma == False`. If there are uncertainties,
# assume they are "absolute" and not "relative".
# For details, see curve_fit:
# https://github.com/scipy/scipy/blob/
# c1ed5ece8ffbf05356a22a8106affcd11bd3aee0/scipy/
# optimize/_minpack_py.py#L591-L602
sum_sqrs = np.sum(self.objective_function(fitparams, *farg) ** 2)
dof = len(y) - len(init_values)
self.fit_info["param_cov"] *= sum_sqrs / dof

Check warning on line 1260 in astropy/modeling/fitting.py

View check run for this annotation

Codecov / codecov/patch

astropy/modeling/fitting.py#L1258-L1260

Added lines #L1258 - L1260 were not covered by tests
else:
self.fit_info["param_cov"] = None

if self._calc_uncertainties is True:
if self.fit_info["param_cov"] is not None:
self._add_fitting_uncertainties(model, self.fit_info["param_cov"])
Expand Down Expand Up @@ -1306,9 +1317,14 @@
z : array, optional
input coordinates
weights : array, optional
Weights for fitting.
For data with Gaussian uncertainties, the weights should be
1/sigma.
Weights for fitting. For data with Gaussian uncertainties, the weights
should be 1/sigma.

.. versionchanged:: 5.3
Calculate parameter covariances while accounting for ``weights``
as "absolute" inverse uncertainties. To recover the old behavior,
choose ``weights=None``.

maxiter : int
maximum number of iterations
acc : float
Expand Down Expand Up @@ -1349,7 +1365,9 @@
model_copy, farg, maxiter, acc, epsilon, estimate_jacobian
)

self._compute_param_cov(model_copy, y, init_values, cov_x, fitparams, farg)
self._compute_param_cov(

Check warning on line 1368 in astropy/modeling/fitting.py

View check run for this annotation

Codecov / codecov/patch

astropy/modeling/fitting.py#L1368

Added line #L1368 was not covered by tests
model_copy, y, init_values, cov_x, fitparams, farg, weights
)

model.sync_constraints = True
return model_copy
Expand Down
36 changes: 35 additions & 1 deletion astropy/modeling/tests/test_fitters.py
Expand Up @@ -619,7 +619,7 @@ def test_param_cov(self, fitter):

with NumpyRNGContext(_RANDOM_SEED):
x = np.linspace(0, 1, 100)
# y scatter is amplitude ~1 to make sure covarience is
# y scatter is amplitude ~1 to make sure covariance is
# non-negligible
y = x * a + b + np.random.randn(len(x))

Expand All @@ -638,6 +638,40 @@ def test_param_cov(self, fitter):
assert_allclose(fmod.parameters, beta.ravel())
assert_allclose(olscov, fitter.fit_info["param_cov"])

@pytest.mark.parametrize("fitter", non_linear_fitters)
def test_param_cov_with_uncertainties(self, fitter):
"""
Tests that the 'param_cov' fit_info entry gets the right answer for
*linear* least squares, where the answer is exact
"""
fitter = fitter()

a = 2
b = 100

with NumpyRNGContext(_RANDOM_SEED):
x = np.linspace(0, 1, 100)
# y scatter is amplitude ~1 to make sure covariance is
# non-negligible
y = x * a + b + np.random.normal(size=len(x))
sigma = np.random.normal(loc=1, scale=0.1, size=len(x))

# compute the ordinary least squares covariance matrix
# accounting for measurement uncertainties `sigma`
X = np.vstack([x, np.ones(len(x))]).T
inv_N = np.linalg.inv(np.diag(sigma) ** 2)
cov = np.linalg.inv(X.T @ inv_N @ X)
beta = cov @ X.T @ inv_N @ y.T

# now do the non-linear least squares fit
mod = models.Linear1D(a, b)

with pytest.warns(AstropyUserWarning, match=r"Model is linear in parameters"):
fmod = fitter(mod, x, y, weights=sigma**-1)

assert_allclose(fmod.parameters, beta.ravel())
assert_allclose(cov, fitter.fit_info["param_cov"])


class TestEntryPoint:
"""Tests population of fitting with entry point fitters"""
Expand Down
2 changes: 2 additions & 0 deletions docs/changes/modeling/14519.api.rst
@@ -0,0 +1,2 @@
Propagate measurement uncertainties via the ``weights`` keyword argument into the
parameter covariances.
12 changes: 12 additions & 0 deletions docs/whatsnew/5.3.rst
Expand Up @@ -19,6 +19,7 @@ In particular, this release includes:
.. * :ref:`whatsnew-5.3-unit-formats-fraction`
.. * :ref:`whatsnew-5.3-unit-formats-order`
.. * :ref:`whatsnew-5.3-nddata-collapse-arbitrary-axes`
.. * :ref:`whatsnew-5.3-modeling-measurement-uncertainties`
.. * :ref:`whatsnew-5.3-coordinates-refresh-site-registry`

In addition to these major changes, Astropy v5.3 includes a large number of
Expand Down Expand Up @@ -198,6 +199,7 @@ we can take the sum of ND masked quantities along the ``1`` axis like so::
array([ True, False]),
StdDevUncertainty([1.41421356, 1.73205081]))


.. _whatsnew-5.3-coordinates-refresh-site-registry:

Refresh cached observatory site registry for |EarthLocation| methods
Expand All @@ -214,6 +216,16 @@ however, is updated from time to time to include new locations. The user
may refresh their locally cached site registry by passing the new
``refresh_cache=True`` option to these two functions.

.. _whatsnew-5.3-modeling-measurement-uncertainties:

Support for collapse operations on arbitrary axes in ``nddata``
===============================================================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too late to fix, but the whatsnew section title is wrong. (Please be more careful in future PRs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! My apologies 😱

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this is super confusing... Is there a way to fix just the doc without doing another release?

https://docs.astropy.org/en/stable/whatsnew/5.3.html#whatsnew-5-3-modeling-measurement-uncertainties

Also, while we're at it, is it normal to have dev tag displayed on the "stable" URL?

Screenshot 2023-05-23 104034

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will be for the next bugfix release, 5.3 is done, but good point, we need to fix that in the v5.3.x branch.

About the dev tag, this is the goal of #14860. (Not a new issue, just managed to find a solution before forgetting about it ;)).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed it directly on v5.3.x 31f3290

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks !


Propagate measurement uncertainties into the best-fit parameter covariances
via the ``weights`` keyword argument in non-linear fitters. Decreasing
the ``weights`` will now increase the uncertainties on the best-fit parameters.


Full change log
===============

Expand Down