Added theta parameter to GaussianKernel2D #3605

[aniketk21]

@astrofrog @embray Please review.
Should I change the documentation for GaussianKernel2D and add an entry to CHANGES.rst?

[eteq]

@aniketk21 - yes, you definitely need to update the docs and add a changelog entry. Note that this should go under the changelog section for v1.2, as it's a new feature.

Also, it looks to me like the discussion in #3605 settled on expecting the arguments to be exactly the same as Gaussian2DModel (see here for the docs on that). So for example, width should be x_stddev. But perhaps @astrofrog or @adonath would like to comment on this?

Otherwise this looks pretty good to me.

[eteq]

Oh, one other worry, though: it's good that this is mostly backwards compatible, but there's a subtle change that the first argument is no longer called stddev. That's a problem if someone is already using it as GaussianKernel2D(stddev=<something>). So perhaps this should be modified to check kwargs for stddev and raise a deprecation warning or something (while still accepting it?)

[aniketk21]

@eteq I'll add the docs and changelog.
Waiting for @astrofrog's and @adonath's comments.

[adonath]

@aniketk21 Thanks for working on this! Yes, definitely add the docs and and a changelog entry. I agree with @eteq to repeat the parameter names from Gaussian2D i.e. x_stddev and y_stddev and check the kwargs for stddev and raise a deprecation warning.

I think the additional support_scaling parameter is not necessary, because the kernels already take x_size and y_size parameters to specify the size of the support.

Optional:
Correctly computing the default size of the support for a tilted elongated Gaussian is a bit tricky, because you actually have to project the x_stddev and y_stddev to the x and y axes to get the correct size of the support. But one could re-use the bounding_box attribute for that. For simplicity, as you already did, you can just take the maximum of the (projected) x_stddev and y_stddev. Ideally you would introduce a hidden _default_y_size attribute and check for that here. I think both points are not very critical, but they shouldn't be much work either.

[aniketk21]

@eteq @adonath Changed the parameter names and added deprecation warning. Also updated the docs and CHANGES.rst #3605
@adonath I'll try the optional part. How do I use the bounding_box attribute?

[astrofrog]

@eteq @adonath - just a general question about these kinds of changes (to stddev) - same question applies for the trapezoid in #4856 - do we really want to have to require the user to tie the parameters if they want to fit a circular gaussian? Or should we have two different models, one with circularly symmetric gaussian, one more general? (requiring users to tie parameters may restrict which fitters can be used?)

[adonath]

@astrofrog I'm not really sure what the best option is. I know that Sherpa avoids this kind of problem, by using a different parametrization of the models, that includes an asymmetry parameter ellipticity (see e.g. beta2d or gauss2d). But I somehow don't like it, because the parametrization it's less intuitive than just specifying minor axes and major axes.

I think in general it's not a big problem, if there are multiple parametrizations of a model available.
E.g. I think it would be worth as well to have an alternative parametrization of some models, where the amplitude parameter corresponds to the integral and not to the amplitude at the peak.

The only issue I see is possible code duplication and a crowded model name space. The later could be avoid with a clever nomenclature of the models. E.g. let the different parametrizations of the model still start with the same model name such as Gaussian2D, Gaussian2DSymmetric or Gaussian2DNormal.

Another option could be introduce a short-cut for direct coupling of parameters, so one could do:

sym_gauss = Gaussian2D(x_stddev=1, tied={'x_stddev': 'y_stddev'})

But this still leaves the issue which fitter can be used.

[eteq]

Hmm, I think I'm slightly in favor of having multiple models i.e., I would say it's reasonable to have one be the subclass of the other - i.e. Gaussian2DCircular is a subclass of Gaussian2D. That would then re-use the same internal machinery. That should allow addressing the duplication @adonath is worried about. It also makes it clearer that the circular one is a more "restricted" version of the non-circular one. I think that's just fine because you would only use a "derived" one if you knew you wanted the restrictions. (This might have been what you meant by Gaussian2DNormal, @adonath? But I wasn't clear what that was supposed to mean perhaps because this is overloaded use of the word "normal"?).

[eteq]

Oh and I'm -1 to saying "only ellipticity" or similar "clever" solutions. Sometimes you really do want to specify the parameters in different ways, and it actually matters. I think it's OK to have multiple models that are really the same thing but have different parameterizations. After all, we have tons of polynomials and they're still all re-parameterizations of the same model...

[astrofrog]

I also think it would make sense to have more model classes, both in e.g. the trapezoid case and gaussian case.

[adonath]

@eteq The Gaussian2DNormal was just an example for another parametrization of a Gaussian, where the amplitude parameter corresponds to the integral and not to the amplitude at the peak, which is useful in many cases, because it resolves the correlation of width and amplitude during the fit.

Adding more models with different parametrizations and explicit names seems to be a good option to me as well.

[eteq]

Ohh, gotcha, yeah that makes sense, although it might be good to avoid Gaussian2DNormal only because Gaussian and Normal mean the same thing. So maybe Gaussian2DNormalized. But that's word-smithing - I like the idea for sure!

[pllim]

The changes do not look very controversial. Was there a reason why this is stalled?

@aniketk21 , do you still wish to wrap this up? You need to:

• Squash the commits.
• Rebase and move the change log to v2.0 section.
• Update related tests to use the new parameters, and also have one of the test still use the deprecated one but check for the warning.

[astropy-bot]

Hi humans 👋 - this pull request hasn't had any new commits for approximately 1 year, 6 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please close this and open an issue instead to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If you believe I commented on this issue incorrectly, please report this here.

[pllim]

This looks potentially useful but abandoned. I might pick this up and finish it but it is pretty far down my to-do list right now. Anyone else interested feel free to take over.

[bsipocz]

Same issue here, it was on my radar, but fallen to the bottom of the todo. (I remember going back to this during pyastro, but then got side-tracked and only got to the point of rebasing it.)

[pllim]

An attempt to continue this work is at #6748