Merge model classes #2276

embray · 2014-04-04T16:54:33Z

Note: This PR builds on top of #2264 and #2269 which should be merged before this one. If either of them is not to be merged this PR's dependency on those other changes can be cleaved off, however.

As pointed out by @nden at https://github.com/astropy/astropy/pull/2149/files#r10402422 , different classes of models had different mechanisms for storing their parameter values, creating a fair amount of unncessary complexity.

This change attempts to smooth out some of the differences by merging the old ParametricModel class into the base Model class, so that all models more or less operate on the same basis. This simplifies the code a fair amount and will make it easier to maintain. An added advantage to this is that it adds full support for parameter constraints on all models, which can be useful even outside the context of fitting. I intend to build that functionality in on top of this PR.

This standardizes on the way ParametricModel stored parameters, where all the parameter values are stored in a single flat array along with a simple _param_metrics data structure providing information how to extract individual parameter values from that array. As @nden points out this may not be the best way. In fact, it might be difficult to really make this work with parameters as Quantities. But this was the path of least resistance for the first iteration. It will also now be much easier to change to a different storage method later, as the changes are only required in a couple places.

For now I've kept around a class named ParametricModel and its subclasses ParametricModel1D and ParametricModel2D and the subclasses thereof--the only thing that really distinguishes them at the moment is the fact that they have the fittable=True flag.

This does not fully resolve the issue addressed in #2149, but it will make the implementation for #2149 simpler. This also adds a few inefficiences, particularly in any models that have angular parameter values that need to be converted to radians for compuations. I have a separate branch running parallel to this one that will attempt to address that issue.

cdeil · 2014-04-04T19:26:08Z

astropy/modeling/core.py

+    @property
+    def parameters(self):
+        """
+        A flattened array of all parameter values in all parameter sets


Add dot at the end of this line.

Not to nitpick about dots, but as a preference I don't use them in places like this since it's not actually a proper sentence.

But if we've decided to make the first line of every docstring a "sentence" regardless I'm okay with it for consistency's sake.

It's actually required for numpydoc I think - we tweaked it so it would use full sentences instead of just first line, so I think it stops at the first full stop for the summary table.

Actually, it doesn't matter if there are no lines after, but I can't see it from here.

That's fine--I don't mind updating it.

cdeil · 2014-04-04T19:38:22Z

@embray Does this need a change log entry?
Do you think ParametricModel should eventually (or in this PR) be removed?
Does this models docs section need an update or is it still good?

embray · 2014-04-04T20:35:59Z

I haven't decided for sure yet, but I might just remove ParametricModel altogether in this PR, if no one objects. I might replace it with something called FittableModel but really the only thing that would distinguish it is still having fittable=True and requiring it to have an eval and optionally fit_deriv, etc. I have a separate PR that will even further remove that distinction, but I want to kind of keep things separate for now.

@nden

<https://github.com/astropy/astropy/pull/2149/files#r10402422> that different classes of models had different mechanisms for storing their parameter values, creating a fair amount of unncessary complexity. This change attempts to smooth out some of the differences by merging the old ParametricModel class into the base Model class, so that all models more or less operate on the same basis. An added advantage to this is that it adds full support for parameter constraints on *all* models, which can be useful even outside the context of fitting. This standardizes on the way ParametricModel stored parameters, where all the parameter values are stored in a single flat array along with a simple _param_metrics data structure providing information how to extract individual parameter values from that array. As @nden points out this may not be the *best* way. In fact, it might be difficult to really make this work with parameters as Quantities. But this was the path of least resistance for the first iteration. For now I've kept around a class named ParametricModel and its subclasses ParametricModel1D and ParametricModel2D and the subclasses thereof--the only thing that really distinguishes them at the moment is the fact that they have the `fittable=True` flag. This does *not* fully resolve the issue addressed in astropy#2149, but it will make the implementation for astropy#2149 simpler. This also adds a few inefficiences, particularly in any models that have angular parameter values that need to be converted to radians for compuations. I have a separate branch running parallel to this one that will attempt to address that issue.

initial values are set: * Constraints are treated in two separate categories: 'parameter constraints' which are constraints on specific parameters, and 'model constraints' which are general constraints on the model (specifically the 'eqcons' and 'ineqcons' constraints). * All constraints are stored in the ._constraints dict. * Setting of initial values of constraints is simplified so that only Model._initialize_constraints is responsible for this (though it still queries the individual parameters for any default constraints they may have).

…int better describes the small distinction it has over other Model types. All models (or almost all anyway) have one or more adjustable parameters of some sort or another"

… and isn't even necessary for the current JointFitter implementation. One can see how it might be useful to store somewhere on the model a reference to what other model(s) its params have been fitted against. But for the time being if a user is setting up a JointFitter they already have that information. Could still be worth revisiting though.

cdeil · 2014-04-11T12:18:39Z

astropy/utils/timer.py

@@ -249,7 +249,7 @@ def do_fit(self, model=None, fitter=None, power=1, min_datapoints=3):
        Returns
        -------
        a : array_like
-            Fitted `~astropy.modeling.core.ParametricModel.parameters`.
+            Fitted `~astropy.modeling.core.FittableModel.parameters`.


I think it should be

`~astropy.modeling.FittableModel.parameters`

i.e. without the .core part.

I'm not sure, but this might be the reason for this cryptic Sphinx TypeError:
https://travis-ci.org/astropy/astropy/jobs/22730168#L3045

That's weird--why would it be that? What's there is the correct fully-qualified name of the class, and the ".core" was there previously as well. Nonetheless, that change did seem to fix it for me. Any idea why that is? The error message is hopelessly cryptic, even for me.

cdeil · 2014-04-11T12:19:40Z

I think this travis-ci error in astropy.vo.samp is unrelated ... @embray can you please restart that build?
https://travis-ci.org/astropy/astropy/jobs/22730167#L1846

cdeil · 2014-04-11T12:20:28Z

@embray Is this ready for review or work in progress?

astrofrog · 2014-04-11T12:59:10Z

I thought the SAMP failures had stopped occurring, but they seem to have started again recently...

embray · 2014-04-11T14:47:45Z

IMHO this PR is basically ready soon as the issue with the docs build is sorted out.

I have lots of other parallel work going that will depend on this PR but it will probably be easier to go ahead and submit those as new PRs.

embray · 2014-04-11T14:50:39Z

If there is any one open issue it's the handling of __str__ and __repr__. Right now FittableModel includes a different implementation of those from the default. And I think neither version is in particularly good shape, especially __repr__. __str__ is a little better but only a little. I'm open to ideas on this. FWIW I also have an experimental branch (not online yet) that is standardizing how formulas for models are represented, in such a way that the formulas could be used in the representation as well (particularly in _repr_latex_ context).

cdeil · 2014-04-11T14:51:26Z

In the past year big astropy.modeling PRs sometimes got stuck and didn't get merged for many months ... so 👍 to get this merged soon and continue with extra PRs.

…tead I have to drop the '.core' part. Huh?

embray · 2014-04-14T23:25:26Z

Okay, going ahead with this now that Travis is happy. This might cause some other open PRs to require rebasing but I will work on helping to do that as need be.

Merge model classes

embray · 2014-04-14T23:26:16Z

Ack. This really ought to be in the changelog. Although we have said that the modeling package will be in flux it is still worth mentioning.

Add record of the changes in #2276

Add record of the changes in astropy#2276

embray mentioned this pull request Apr 4, 2014

Implement custom_model_2d #1763

Merged

cdeil reviewed Apr 4, 2014
View reviewed changes

embray mentioned this pull request Apr 4, 2014

Make AffineTransformation2D fittable #2277

Closed

astrofrog added the modeling label Apr 10, 2014

astrofrog added this to the v0.4.0 milestone Apr 10, 2014

embray added 5 commits April 10, 2014 17:06

Docstring tweaks as suggested by @cdeil

095b817

Rename Parametric.*Model to Fittable.*Model--a name whicih at this po…

3187719

…int better describes the small distinction it has over other Model types. All models (or almost all anyway) have one or more adjustable parameters of some sort or another"

cdeil reviewed Apr 11, 2014
View reviewed changes

Apparently this won't resolve if I give the fully class name, and ins…

b68362b

…tead I have to drop the '.core' part. Huh?

cdeil mentioned this pull request Apr 12, 2014

Non-deterministic test failures in vo.samp #2064

Closed

embray added a commit that referenced this pull request Apr 14, 2014

Merge pull request #2276 from embray/merge-parametric-model

af66877

Merge model classes

embray merged commit af66877 into astropy:master Apr 14, 2014

embray deleted the merge-parametric-model branch April 14, 2014 23:26

embray added a commit that referenced this pull request Apr 14, 2014

Update CHANGES.rst

46b4e39

Add record of the changes in #2276

ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014

Update CHANGES.rst

a9ae2fd

Add record of the changes in astropy#2276

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge model classes #2276

Merge model classes #2276

embray commented Apr 4, 2014

cdeil Apr 4, 2014

embray Apr 4, 2014

astrofrog Apr 4, 2014

astrofrog Apr 4, 2014

embray Apr 7, 2014

cdeil commented Apr 4, 2014

embray commented Apr 4, 2014

cdeil Apr 11, 2014

embray Apr 11, 2014

cdeil commented Apr 11, 2014

cdeil commented Apr 11, 2014

astrofrog commented Apr 11, 2014

embray commented Apr 11, 2014

embray commented Apr 11, 2014

cdeil commented Apr 11, 2014

embray commented Apr 14, 2014

embray commented Apr 14, 2014

Merge model classes #2276

Merge model classes #2276

Conversation

embray commented Apr 4, 2014

cdeil Apr 4, 2014

Choose a reason for hiding this comment

embray Apr 4, 2014

Choose a reason for hiding this comment

astrofrog Apr 4, 2014

Choose a reason for hiding this comment

astrofrog Apr 4, 2014

Choose a reason for hiding this comment

embray Apr 7, 2014

Choose a reason for hiding this comment

cdeil commented Apr 4, 2014

embray commented Apr 4, 2014

cdeil Apr 11, 2014

Choose a reason for hiding this comment

embray Apr 11, 2014

Choose a reason for hiding this comment

cdeil commented Apr 11, 2014

cdeil commented Apr 11, 2014

astrofrog commented Apr 11, 2014

embray commented Apr 11, 2014

embray commented Apr 11, 2014

cdeil commented Apr 11, 2014

embray commented Apr 14, 2014

embray commented Apr 14, 2014