astropy/astropy — pull request #1086 by iguananaut (2013-05-13 17:59:06+00:00)
Title: New parameter handling for models, and other changes

Here's an initial look at my proposed changes to the Model interface that I've made references to in #980. This is by no means final or complete, but it does have all the (existing) tests in master passing. I haven't tried it with any of the additional tests being added in #1060 yet, and I suspect many of them would fail currently.

I had two main goals behind this work, though it turned out to be even more pervasive than I'd anticipated, so several other changes came along for the ride:

1. I thought it was sort of problematic that the `__init__` for models was mutating model *classes* (in effect, global state) every time a model instance was created, by attaching new `_ParameterProperty` descriptor objects to the class. This was mostly harmless, as each model class had a fixed set of parameters. But it's still a strange thing to do in general, and could lead to odd behavior in particular with `PolynomialModel` objects (where the class has parameters defined on it for coefficients that aren't defined for a particular model instance). So I wanted to fix that.

2. I also found it confusing that to fully define a Model and all its parameters required an interplay between three very closely intertwined objects (four if you consider `Constraints` objects as well). These include subclasses of `Model` itself, `Parameter` objects, and the `Parameters` container. I found that trying to keep these three classes of objects synchronized was making the code more complicated (especially in the Fitters) and contributed to the complexity of defining new models.

This PR contains several changes, so allow me to summarize:

* There is a lot of PEP-8-related whitespace cleanup and things of that nature. Most of that is done automatically by my editor the way it's configured, so I apologize that it distracts from the significant changes. From the command-line git will allow you to view a diff without whitespace changes--I don't know if github has such a feature.

* Significant modifications have been made to the `Parameter` and `Parameters` classes so that rather holding their own copies of parameter values, all parameter values are held by the Model alone, and the `Parameter` and `Parameters` classes merely provide specialized views of the parameter values.

* The `_ParameterProperty` and `Parameter` classes have been combined into a single class. If this is decided to be too confusing they can still be broken into separate classes again, but I wanted to make them into a single class because I like that they're both called just "Parameter". More on the dual purpose and usage of the `Parameter` class below.

* The `Parameters` class is also somewhat simpler (though I would like to make it simpler still, or possibly get rid of it entirely and have the model store all its parameters in an array by default).

* Some of the functionality shared between the `PolynomialModel` and `OrthogPolyBase` classes was broken out into a separate `PolynomialBase` class from which `PolynomialModel` inherits. For better consistency `OrthogPolyBase` was renamed `OrthogonalPolynomialModel` (though if people don't like that it can be changed back no problem) and it also inherits from `PolynomialBase`.

* Several models that took parameters as keyword arguments had the `**pars` argument renamed to `**parameters`. Again, trying to be more consistent with abbreviating vs. not abbreviating.

* Shared functionality/parameters in the `RotateNative2Celestial` and `RotateCelestial2Native` models was broken out into a separate, simpler `EulerAngleRotation` class. The new `Parameter` interface also made this class much simpler (through the use of parameter getters/setters).

Sorry again for the big cluster of changes. Some of them had to come together, but others are kind of superfluous and I can easily back them out if they're not found to be preferred.

The new `Parameter` class serves a dual purpose and requires some explanation:

* It works as a descriptor just like the `_ParameterProperty` did previously. But now (with the exception of most polynomial models which I'll discuss below) implementations of new `Model` subclasses should list each of its properties in the class definition like so:

```
class MyModel(object):
parameter_a = Parameter('parameter_a')
parameter_b = Parameter('parameter_b')
# etc...
```
This is *instead* of using the `param_names` list. Now, this may seem redundant because you're assigning a `Parameter` object to some class attribute and giving the parameter a name that's the same as the class attribute. But this use of `Parameter` descriptors is not yet taken to its full advantage. The `Parameter()` call could also, for example, provide a detailed description of that parameter, and other metadata about the parameter (required units, default constraints, etc.). It also already supports `getter` and `setter` arguments that add custom wrappers to the values set to properties. For example some of the rotation models take advantage of this to convert between degrees/radians.

* `Parameter` also works, somewhat as it did before, as a wrapper for the actual parameter value. For example when accessing the `g1.amplitude` parameter on a Gaussian1D model a `Parameter` object is returned. This uses the same class as the `Parameter` descriptor, but the difference is that it is "bound" to a model instance. What that currently means implementation-wise is that the `Parameter` instance stores are reference to the `Model` instance that's bound to as an internal attribute, and can use it internally. This is very different from the previous implementation, where the model's `._amplitude` attribute was actually a `Parameter` instance that contained the parameter value(s). Now the `_.amplitude` attribute *just* contains the unadorned value(s) for the parameter, but when a users accesses `.amplitude` (without the underscore) this returns the `Parameter` object that has a name and constraints, and is *bound* to the model, and that's how it knows its "value".

So in summary, unlike the existing interface that requires manually instantiating lots of `Parameter` objects when implementing a new model, most of that work is hidden away, and instead one just assigns values directly to `self.`. If this is unclear, just take a look at how all the built-in Model classes are implemented now compared to before.

One thing that this does not yet support but could easily support is assigning a `Parameter` of one model to a parameter of a different model in such a way that it copies all the constraints as well as the value. But this functionality could be easily added back in.

I also haven't added fully working support for constraints yet, but only because there are very few unit tests for them. I think once #1060 is merged I can work on getting that working--it should be straightforward.

Finally one more word about polynomial models: They are different from the other model classes in that they don't have a fixed set of parameters defined at the class-level. Instead `PolynomialBase` implements `__getattr__` and `__setattr__` to support variable sets of parameters rather than going through descriptors, but the end result is the same.