Linear1D and Planar2D should be able to fit as model sets #10623
Conversation
nden
force-pushed
the
model-sets-linear1d
branch
from
e4f7078
to
207e15c
Compare
| @@ -969,6 +970,8 @@ astropy.modeling | |||
| without units when the expression involves operators other than addition | |||
| and subtraction. [#10415] | |||
|
|
|||
| - Fixed a problem with fitting ``Linear1D`` and ``Planar2D`` ain model sets. [#10623] | |||
|
|
|||
docs/modeling/models.rst
Outdated
| in the correct shape. The shape depends on the ``model_set_axis`` attribute of the | ||
| model to be fit. The rule is that the dependent variable for each model in a set should | ||
| be along the ``model_set_axis`` dimension. For example, for a 1D model set with 3 models | ||
| with ``model_set_axis == 1`` the shape of ``y`` should be (x, 3):: |
There was a problem hiding this comment.
I don't know for sure, but I find the use of 'dependent' variable in this description to be confusing, Dependent on what? The input x values or the model set? I think it should be more specific, something like "the index of the dependent variable that corresponds to the model set should be the model_set_axis, for example, if there were 10 x input values and 3 model sets the shape of y should be (10, 3). Or something like that.
There was a problem hiding this comment.
Thanks, I was struggling with this sentence.
docs/modeling/models.rst
Outdated
| >>> m = Polynomial1D(1, n_models=3, model_set_axis=1) | ||
| >>> mfit = fitter(m, x, y) | ||
|
|
||
| For a 2D model set with ``model_set_axis == 0`` the shape of ``z`` should be (2, x, y):: |
There was a problem hiding this comment.
Here 2D is potentially misunderstood. Perhaps reword as "For 2D models with the model_set_axis = 0" (should we really use '==' instead of '='?)
| >>> print(z.shape) | ||
| (2, 2, 4) | ||
| >>> m = Polynomial2D(1, n_models=2, model_set_axis=0) | ||
| >>> mfit = fitter(m, x, y, z) |
There was a problem hiding this comment.
I always figure it is good to avoid dimensions that are identical in size to prevent confusion. Here we have two that are 2.
| fixderivs = self._deriv_with_constraints(model_copy, | ||
| fixparam_indices, | ||
| x=x) | ||
| lhs = np.asarray(self._deriv_with_constraints(model_copy, |
There was a problem hiding this comment.
Just to understand better, what was being returned before?
There was a problem hiding this comment.
Previously, it was a list.
|
|
||
| with pytest.raises(ValueError): | ||
| p1(x) | ||
|
|
||
| with pytest.raises(ValueError): | ||
| p1(xx) | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
cut and paste error, removed it
| @@ -204,11 +210,43 @@ def test_shapes(): | |||
| assert t.e.shape == (2,) | |||
|
|
|||
|
|
|||
| def test_linearlsqfitter(): | |||
| def test_eval(): | |||
| """ Tests evaluation of Linear1D and Planar2D with differrent model_set_axis.""" | |||
|
@perrygreenfield I addressed your comments. Thanks. |
| @@ -495,9 +494,6 @@ def __call__(self, model, x, y, z=None, weights=None, rcond=None): | |||
| lhs *= weights[:, np.newaxis] | |||
| rhs = rhs * weights | |||
|
|
|||
| if rcond is None: | |||
| rcond = len(x) * np.finfo(x.dtype).eps | |||
|
|
|||
There was a problem hiding this comment.
To explain: numpy now deals with this, if rcond is None.
nden
force-pushed
the
model-sets-linear1d
branch
from
c4dcbd0
to
c228476
Compare
Linear1D and Planar2D should be able to fit as model sets
Linear1D and Planar2D should be able to fit as model sets
Linear1D and Planar2D should be able to fit as model sets
Linear1D and Planar2D should be able to fit as model sets
Linear1DandPlanar2Dshould be able to fit as model sets withLinearLSQfitter which this PR enables.While fixing the problem I noticed a larger (design) issue with the way the fitter calls the derivative of a model. Currently the call is
This works fine with single models. However, the signature of this method for non-linear models lists all parameters as keyword parameters. When a model set is fit the list
*model.parametersbecomes longer than the list of parameters in the signature.When fitting.py is refactored the call to
fit_derivneeds to be changed to solve this problem.