More Gauss model changes

@adrn

@adrn @nden @eteq After #1022 you are probably tired of discussing the Gauss model classes, but I just tried them out and would like to make further changes.

Given that the Gauss models are our test case before adding plenty of other models in #981 I think this is important. Once #980 is done I'd be happy to implement the changes we decide on here.

I propose the following changes:

This one is uncontroversial, I think. :-)
Fix the Gaussian2DModel model evaluation, it currently gives incorrect results:

In [55]: from astropy.modeling import models

In [56]: g = models.Gaussian2DModel(amplitude=42, x_mean=0, y_mean=0, x_stddev=1, y_stddev=1)

In [57]: g(0, 0)
Out[57]: nan

In [58]: g = models.Gaussian2DModel(amplitude=42, x_mean=43, y_mean=44, x_stddev=1, y_stddev=1)

In [60]: g(43, 44) # Should return amplitude = 42
Out[60]: 6.2250310471769028

The order of stddev and fwhm in the constructor should be changed. We decided that that stddev is the internal Parameter and is listed in the Gauss param_names = ["amplitude", "mean", "stddev"] attribute, so this is what I expect to set to stddev = 3 when calling models.Gaussian1DModel(1, 2, 3). Currently the order in the constructor is fwhm before stddev :
models.Gaussian2DModel(self, amplitude, x_mean, y_mean, x_fwhm=None, y_fwhm=None, x_stddev=None, y_stddev=None, ...)

Did we want the fwhm property? I thought from the discussion in Renamed parameters in Gaussian and added a cov_matrix kwarg #1022 we did, but it's currently not there:

In [63]: g = models.Gaussian1DModel(1, 2, fwhm=3)

In [64]: g.fwhm
ERROR: AttributeError: 'Gaussian1DModel' object has no attribute 'fwhm' [IPython.core.interactiveshell]
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-64-455d10d24dbd> in <module>()
----> 1 g.fwhm

AttributeError: 'Gaussian1DModel' object has no attribute 'fwhm'

I think personally I would actually be in favour to remove fwhm completely (also from the constructor). Typically users are also interested in fit parameter errors and the covariance matrix will only contain the parameter error and correlation with other parameters for stddev, so if the user wants fwhm she has to convert the error herself anyways. Of course it is possible to simultaneously support fwhm in addition to stddev for errors (and ties?), but it makes the class more complicated to write and learn and I think asking users that want fwhm to do a unit conversion of their input and output would be the better solution. What do you think?
I would like to change the name back to Gauss instead of Gaussian. This would be consistent with Chebyshev and Legendre, where we also don't use Chebyshevian and Legendrian (probably I misspelled that). I don't see any disadvantage to using the shorter Gauss.
This is pure boiler-plate code and should be moved to some base class so that we don't have to copy & Paste it to every model:

    def __call__(self, x):
        """
        Transforms data using this model.

        Parameters
        --------------
        x : array, of minimum dimensions 1

        Notes
        -----
        See the module docstring for rules for model evaluation. 
        """
        x, fmt = _convert_input(x, self.param_dim)
        result = self.eval(x, self.param_sets)
        return _convert_output(result, fmt)

I agree with most of these ideas, but I personally think we should change the other models to have longer names for consistency, not shorten Gaussian. That is, Poly -> Polynomial etc.

I also am in favor of removing fwhm.

I was in favor of keeping fwhm as it is commonly used but you have a very good point about errors. Correctness trumps convenience so I am fine with removing fwhm.

I am flat 0 on any name changes. I do prefer shorter names but won't object longer ones unless someone comes up with a name like PafnutyChebishevPlolynomialOf1stKind1DModel.

The rest of the changes look reasonable.

Don't know why I feel so strongly about this, haha, but I do! We would never say Chebyshevian polynomial because they are named Chebyshev polynomials. The Gaussian function is named the Gaussian function.

See: http://en.wikipedia.org/wiki/Gaussian_function vs. http://en.wikipedia.org/wiki/Chebyshev_polynomials

Or, maybe a better argument, we can be consistent with Scipy's names:
http://docs.scipy.org/doc/scipy/reference/generated/scipy.signal.gaussian.html
and
http://docs.scipy.org/doc/numpy/reference/routines.polynomials.chebyshev.html

Maybe 'Polynomial' could be short, but the name of the polynomial could be long? I.e. ChebyshevPoly (since Poly will be used in several places and is pretty obvious)

@adrn

3/4: I don't understand the argument for not having fwhm. What is the disadvantage of including it? I agree we don't want the fit statistics and such to be duplicated for fwhm, but what's wrong with having a property derived from stddev or being able to initialize it that way? It's just a simple scaling it all cases. And it significantly improves readability over forcing people to scale by a factor all the time. Remember that modeling is not just about the fitting - users will also want to use the models themselves.

I do agree it shouldn't be first as a positional argument, though. In fact, when I think about it, it probably makes sense to have only one representation available as positional arguments (probably mean and sigma), and the others only available as keyword arguments.

5: I'm with @adrn on longer names in general. At least for me, coding is not dominated by typing class names, but rather trying to figure out what various things do. So longer names are more efficient, not less.

That said, I'm -0 on ChebyshevPoly over ChebyshevPolynomial. Just Poly would be unacceptable on it's own, as it's ambiguous, so we need Polynomial, so ChebyshevPolynomial is a natural follow-on. But it is quite long, and ChebyshevPoly is unambiguous so it's not that big a deal.

The rest all looks fine to me.

@eteq

@eteq - I also don't feel strongly about it, so longer names are fine for me too.

One more note: for math models like Gauss or PowerLaw (see issue #1048) we should put the formula in the docstring (sometimes there's different common parametrizations and user's shouldn't have to look at eval to be sure).

If possible it could also be nice to clearly separate model fit parameters from other parameters the constructor takes. This is something that is at the moment a bit nicer in the Sherpa docs compared to the astropy docs, where e.g. the jacobian_func parameter appears together with amplitude, mean and stddev. Maybe we can auto-generate a little table (parameter name, short parameter description) like in the Sherpa docs?

@cdeil

@cdeil - 👍 on putting the formulae in all of the models where it makes sense (i.e. for the various orthogonal polynomials we don't want to put in the formulae explicitly, because they quickly get lengthy and there's no obvious order at whch to stop.)

Note that I've already done this with sphinx-renderable markup for all the models at http://pythonhosted.org/PyModelFit/builtins.html (see e.g. http://pythonhosted.org/PyModelFit/builtins/pymodelfit.builtins.GaussianModel.html#pymodelfit.builtins.GaussianModel ), which has quite a bit of overlap with modeling. If someone else wants to copy and paste from the pymodelfit docs, they should absolutely feel free.

By the way, if you put a _repr_latex_ method on each one it wll also render their formulae in IPython notebook.

Oh, and regarding having an optional 'short' parameter name, that would work pretty easily in a framework like I've suggested in #1086. That could even support having attributes for both versions. For example g.amplitude and g.A would return the same parameter. The Parameter object could even be given a latex symbol associated with it like \mu or \sigma for use in generating the docs.

+1 to that!

Merge pull request #1106 from astrofrog/modeling/fix-gaussian-stddev

@cdeil

@cdeil As far as I can tell most of the problems in the original issue have been implemented in various PRs. The only thing left is adding the formula to the docstrings of a few models. This can be done in a separate PR so I suggest we close this one.

closing this, documentation fixes will go in a different PR

Navigation Menu

Repository navigation

More Gauss model changes #1077

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

More Gauss model changes #1077

Description

Activity

adrn commented on May 10, 2013

nden commented on May 10, 2013

adrn commented on May 13, 2013

astrofrog commented on May 13, 2013

eteq commented on May 14, 2013

astrofrog commented on May 14, 2013

cdeil commented on May 15, 2013

eteq commented on May 15, 2013

embray commented on May 16, 2013

adrn commented on May 16, 2013

nden commented on Sep 30, 2013

nden commented on Oct 8, 2013

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions