Add modeling parameter descriptions (#11230) #11232
Conversation
|
I have submitted this pull request just to get an idea. Basically what I fill be doing to fix this bug is going to every model and adding |
pllim
changed the title
|
Hello and thank you! This isn't really a bug, but more of implementing something new, so I adjusted the title, milestone, and so on. I also convert this to draft status for now. FYI. |
|
Thankyou @pllim , should I continue to fill |
|
@Akshat1Nar , let's wait a bit so you don't waste your time if there is no buy-in. I added checkboxes above for clarity. |
|
I think this looks good. I'm not a maintainer of this sub-package, but @nden has already agreed that this would be good to have in #11230 (comment). I think the level of detail it just right. Of course, for the Gaussian it's pretty easy (I mean, what else could a parameter that's called "amplitude" mean, if not the "Amplitude of the Gaussian"?). I think it's too early to pick on individual words, but I wonder if it would be more useful to explain "mean" as something like "position of the peak"? Saying "the parameter called mean is the mean" is true, but not terribly helpful. So, I suggest that you go ahead and add descriptions for a few more models. I suggest that we look at it again when you have done may 10-12 models. That way, we have a little more to discuss, but we also make sure that we don't waste your time. Thanks you so much for helping everyone making astropy better! |
|
Thankyou @hamogu for that information. I will come back with more descriptions and then you can review again for mistakes. I have also updated first checkbox for now. :) |
There was a problem hiding this comment.
I left some suggestions. First, thanks for doing this. Please look at my suggestion and comments and see what you think. Please keep in mind that they are just that - suggestions that you can take or leave. You'll also see that in some cases my suggestions differ from what's in the docstring for that model. If you think your or my text for the parameter description is better than what's in there currently, feel free to improve the docstrings as well!
I know from personal experience that this part of the process can be very frustrating with you doing the work and me suggesting something else - and possible another reviewer coming in later who might have other ideas. Having these discussions over small details is how we try to make astropy perfect ;-)
| y_mean = Parameter(default=0, description="Average value of random variable(along y axis) having this distribution(Gaussian)") | ||
| x_stddev = Parameter(default=1, description="Standard deviation of the Gaussian (along x axis)") | ||
| y_stddev = Parameter(default=1, description="Standard deviation of the Gaussian (along y axis)") | ||
| theta = Parameter(default=0.0, description="Rotation angle in radians (Optional parameter)") |
There was a problem hiding this comment.
Does it have to be in radians or does the unit framework allow specification in any angle unit?
There was a problem hiding this comment.
It is mentioned in documentation that angle is in radian.
There was a problem hiding this comment.
If provided as quantity, it takes any angle unit as it should; maybe for brevity it can be put in parenthesis as suggested here. I have already updated the main docstring in #11177, but perhaps that could also be further clarified to something like
theta : float or `~astropy.units.Quantity`, optional.
Rotation angle (angular quantity or value in radians). The rotation angle increases
| @@ -506,7 +506,7 @@ class Scale(Fittable1DModel): | |||
|
|
|||
| """ | |||
|
|
|||
| factor = Parameter(default=1) | |||
| factor = Parameter(default=1, description="Factor by which to scale a coordinate") | |||
There was a problem hiding this comment.
| factor = Parameter(default=1, description="Factor by which to scale a coordinate") | |
| factor = Parameter(default=1, description="Factor by which to scale a model") |
|
Hello @Akshat1Nar 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style. There are no PEP8 style issues with this pull request - thanks! 🎉 Comment last updated at 2021-05-06 15:53:46 UTC |
|
@hamogu I took almost all of the suggestions, they all were necessary. And pushed the changes again. Please review and should I work on models in other files as well. This bug seems to be good-first-bug for me since I am getting to know the codebase as well. :) |
|
Looks good. Do you want to continue? |
Yes |
|
@hamogu Please review, I think I have added description to most of them. |
|
@hamogu Review ping. |
|
@Akshat1Nar : Sorry, I have to finish something myself this week. This will have to wait till Friday afternoon or the weekend. |
|
Thankyou for the response @hamogu :) |
|
@Akshat1Nar , sometimes a reviewer needs a few weeks or even months to circle back. Unfortunately, that is the current reality of FOSS workflow. Thank you for your patience! |
|
@Akshat1Nar But we should aim to keep it shorter. Months is far too long in my opinion. If it takes that long, please remind! |
|
Re: OpenAstronomy GSOC participation -- There is a |
|
@pllim Thankyou, I will try to take any upcoming help on gsoc from there :) |
|
Since this hasn't made the feature freeze cutoff I'm re-milestoning to 5.0 |
|
Uuups. I forgot about that. |
|
Unfortunately, one change is that we''re now using town-crier instead of the Changelog file: https://docs.astropy.org/en/latest/development/workflow/development_workflow.html#add-a-changelog-entry Since that's really me and @dhomeier that let it linger for so long, I can fix that up when everything else is done. |
|
The other conflict is east, and I can also take care of that. What this means is, that there is no need to rebase right now, we can review and see if any more changes to the content are required and then re-base at the very end. |
|
Hello @hamogu . What do I need to do to help to complete this pull request? |
|
@Akshat1Nar Do you want to try the doc changes or do you want me to do that? It's kind of a special situation of a PR that was opened before we switched to towncrier and so needs to be updated to that. Instructions are here: #11279 (comment) If you want to do that, you are welcome to, if not, I'll do it in the next few days (adding on top of your commits so that you still get full credit for what you did) |
Co-authored-by: Derek Homeier <dhomeie@gwdg.de>
|
OK, I changed to the CHANGES to the towncrier format and force-pushed that on top of @Akshat1Nar changes. Assuming that the CI still passes, I think this is ready for final review (which is why I added the "ready-for-final-review" label ;-) |
|
@hamogu , does this mean the two reviews that requested changes can be dismissed? Thanks! |
|
@dhomeier asked to review again after this, and I'll look through it, too. However, this is a case of "perfect is the enemy of good". The new description is better then what we had before (nothing), so cases where we've looked at that and decided that it's no ideal, but it mirrors the docstring should, in my opinion, not hold this up. Of course, I'd like to hear from @nden if we can. |
|
@dhomeier I believe that all the changes you requested were already done by @Akshat1Nar , but github did not update the display; not sure why. I've checked and manually marked them as resolved. |
|
Thanks, yes I believe they had been marked resolved earlier this week, but maybe got lost again in the rebase. |
Description
This pull request is to address ...
Fixes #11230
TODO