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

Document rot_wrt_axis parameter #84

Merged
merged 1 commit into from
Aug 3, 2016
Merged

Conversation

cdeil
Copy link
Member

@cdeil cdeil commented Aug 2, 2016

This is a minor cleanup to make rot_wrt_axis default keyword arguments bool instead of int, to make it clearer that this is a flag (and not e.g. a float rotation angle).

@leejjoon - OK?

@cdeil cdeil added this to the v1.2 milestone Aug 2, 2016
@coveralls
Copy link

coveralls commented Aug 2, 2016

Coverage Status

Coverage remained the same at 50.714% when pulling 876cebf on cdeil:rot_wrt_axis into 1304632 on astropy:master.

@leejjoon
Copy link
Contributor

leejjoon commented Aug 2, 2016

'rot_wrt_axis' is from the PR (#34) by @mcara. And I guess he is using this with value of 2. So, this may affect his codes. And the name of the keyword means (I guess) "rotation with respect to axis" 1 or 2, so I guess 1 or 2 makes more sense that True/False. So, unless @mcara is supportive of this PR, I would rather leave it as is, but add an adequate documentation.

@cdeil cdeil changed the title Make rot_wrt_axis bool instead of int Document rot_wrt_axis parameter Aug 3, 2016
@coveralls
Copy link

coveralls commented Aug 3, 2016

Coverage Status

Coverage decreased (-0.05%) to 50.555% when pulling 7516fb2 on cdeil:rot_wrt_axis into 34ebb82 on astropy:master.

@cdeil
Copy link
Member Author

cdeil commented Aug 3, 2016

@leejjoon @mcara - Thanks for the infos.

I've changed the commit here to document rot_wrt_axis as you explained:

        rot_wrt_axis : {1, 2}
            Use rotation with respect to axis 1 (X-axis) or axis 2 (Y-axis) and north.

I've also changed the use to raise a ValueError if the user passes something other than 1 or 2.

@cdeil cdeil merged commit be364da into astropy:master Aug 3, 2016
@cdeil cdeil added docs and removed enhancement labels Aug 3, 2016
@cdeil cdeil mentioned this pull request Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants