Skip to content
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

Definitions of alpha and gamma in Moffat functions #7733

Open
rjavila opened this issue Aug 13, 2018 · 10 comments
Open

Definitions of alpha and gamma in Moffat functions #7733

rjavila opened this issue Aug 13, 2018 · 10 comments

Comments

@rjavila
Copy link

rjavila commented Aug 13, 2018

The documentation in the Moffat1D and Moffat2D models use alpha and gamma, but as far as I can tell, everywhere else these two parameters are defined as beta and alpha, respectively. For example equation 1 in this paper https://arxiv.org/pdf/astro-ph/0109067.pdf

Can the code and documentation be changed to reflect this? It has confused me bit while using the models. Thanks.

@larrybradley
Copy link
Member

That's unfortunate. We can't simply swap them because it would break existing code that uses them. We could possibly deprecate the current alpha and gamma parameters, renaming them to some intermediate names, and then deprecate the intermediate names in favor of the correct alpha and gamma parameters. That process would require at least two release cycles.

@bsipocz
Copy link
Member

bsipocz commented Aug 13, 2018

IMO relying on names of variables can cause problems on many levels, but if the documentation clearly states what each parameter means then this ship has sailed.

@nden
Copy link
Contributor

nden commented Aug 13, 2018

I think it's going to be very confusing to change the names now since the model has been around for about 5 years.

@rjavila
Copy link
Author

rjavila commented Aug 13, 2018

Yeah, I understand that it would be a lot of work to make the code changes. I would say though that the definitions of alpha and beta have been around much longer than 5 years and this can cause confusion for a user that is expecting parameters to be defined in some way. No one would be ok if we started using Phi instead of sigma for standard deviation right?

How about a warning or notification in the documentation to let users know if the difference?

@nden
Copy link
Contributor

nden commented Aug 13, 2018

I am concerned about the impact such a change would have on other code written in these 5 years. It's also unclear to me why those names were chosen to start with. These two models started as Beta models and were renamed to Moffat later but the original parameter names were kept. Is there a context in which the current names are correct or was this an oversight?

@eteq
Copy link
Member

eteq commented Aug 14, 2018

A quick search on github reveals that indeed there are several repositories out there that use Moffat1D (and are not just clones of Astropy). That suggests even more are out there not on github. That goes in the direction of what @larrybradley and @bsipocz said that we don't want to just change this.

That said, I'm almost certain it's what @nden said: alpha/beta come from the beta model, which has essentially the same form. So there is an argument simply about consistency there.

There is a middle-ground though. We could imagine writing some hacks into Moffat1D and Moffat2Dthat make the "right" names (what @rjavila suggests) "act like" the current parameter names - i.e., they could be used interchangably. The problem there is the alpha/alpha collision. Perhaps some name like .moffat_alpha or .no_really_i_mean_that_alpha 😉?

The above suggestion aside, I agree with @rjavila that it's a good idea to point this out somewhere clearly in the docs. @rjavila do you think you can make a PR to do this? The docs for specific models are generated directly from the docstrings - e.g. for 1D: https://github.com/astropy/astropy/blob/master/astropy/modeling/functional_models.py#L2194 - you could add a paragraph with whatever you think might have addressed this problem for you when you were reading the docs? (This is a great example of "developers shouldn't write docs, users should" 🙃). You could also put it in a Notes section of that docstring if you think that would have gotten your attention better.

@rjavila
Copy link
Author

rjavila commented Aug 14, 2018

@eteq solution of a hack would be sufficient for me as a user. I'll let you decide what to do about that.

On the documentation side, I'd be happy to write something up.

@pllim
Copy link
Member

pllim commented Aug 17, 2018

Re: hack -- I attemped to "hack" Gaussian model once and got rejected (#4936). Should be we consistent in the no-hack policy? 😉

@hamogu
Copy link
Member

hamogu commented Nov 20, 2018

We might make a new model Moffat1DWithBetterParameterNames. That model only needs a few lines of code - just call Moffat1D with the right parameters. The very existence of the second model will make users check the docs when they see it and then the dos will have to explain the difference. That way, current users of Moffat1D don't have to do anything and new users who want to use the right names simply use the new model. It's more duplication than the hack suggested above, but has much less potential for confusing the parameters.

@larrybradley
Copy link
Member

@hamogu I would even go further and suggest that after adding the "correct/better" model, that we deprecate Moffat1D/2D. (insert runs for cover emoji here) 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants