-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
More Gauss model changes #1077
Comments
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 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 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: |
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) |
3/4: I don't understand the argument for not having 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 The rest all looks fine to me. |
@eteq - I also don't feel strongly about it, so longer names are fine for me too. |
One more note: for math models like 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 |
@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 |
By the way, if you put a 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 |
+1 to that! |
Gaussian1dModel.stddev is incorrectly used This also fixed points 1 and 2 in #1077
@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 |
@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:The order of
stddev
andfwhm
in the constructor should be changed. We decided that thatstddev
is the internalParameter
and is listed in the Gaussparam_names = ["amplitude", "mean", "stddev"]
attribute, so this is what I expect to set tostddev = 3
when callingmodels.Gaussian1DModel(1, 2, 3)
. Currently the order in the constructor isfwhm
beforestddev
: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: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 forstddev
, so if the user wantsfwhm
she has to convert the error herself anyways. Of course it is possible to simultaneously supportfwhm
in addition tostddev
for errors (and ties?), but it makes the class more complicated to write and learn and I think asking users that wantfwhm
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 ofGaussian
. This would be consistent withChebyshev
andLegendre
, where we also don't useChebyshevian
andLegendrian
(probably I misspelled that). I don't see any disadvantage to using the shorterGauss
.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:
The text was updated successfully, but these errors were encountered: