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

Fixed typos + autopep8 #273

Merged
merged 2 commits into from
Nov 26, 2013
Merged

Fixed typos + autopep8 #273

merged 2 commits into from
Nov 26, 2013

Conversation

samuelstjean
Copy link
Contributor

Found some small typos when viewing docs, so I corrected them and ran an autopep8 on the file as well.

""" Rodriguez formula

Rotation matrix for rotation around axis r for angle theta.

The rotation matrix is given by the Rodrigues formula:
The rotation matrix is given by the Rodriguez formula:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would think, but it doesn't seem so: http://en.wikipedia.org/wiki/Rodrigues'_rotation_formula

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which also suggests that we should change it everywhere else...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though there was one typo, not the opposite. The problem is that correcting it will also change the function name, breaking any custom script function using it. We can change it seems seldom used I guess.

@arokem
Copy link
Contributor

arokem commented Nov 25, 2013

This all looks good to me, except for the Rodrigues/Rodriguez spelling
debacle. Also, Travis seems to be having some issues, so if you make
another commit, maybe he will start behaving.

On Mon, Nov 25, 2013 at 1:09 AM, Samuel St-Jean notifications@github.comwrote:

Found some small typos when viewing docs, so I corrected them and ran an

autopep8 on the file as well.

You can merge this Pull Request by running

git pull https://github.com/samuelstjean/dipy typos

Or view, comment on, or merge it at:

#273
Commit Summary

  • Fixed typos + autopep8

File Changes

Patch Links:

@MrBago
Copy link
Contributor

MrBago commented Nov 25, 2013

This is a little off topic, but how do you guys feel about thinning this module? The following functions are not tested in dipy.core.test.test_geometry and are not called at all during any other dipy tests:

compose_matrix
decompose_matrix
euler_matrix
lambert_equal_area_projection_cart
normalized_vector
rodriguez_axis_rotation

The following functions are tested in dipy.core.tests.test_geometry, but are not called by other dipy test:

cart_distance
circumradius
lambert_equal_area_projection_polar
nearest_pos_semi_def
sphere_distance
vector_cosine

@samuelstjean
Copy link
Contributor Author

I also grepped the whole dipy tree and the function doesn't seems to be imported anywhere. So it will only break user's script, which is bad if anyone was using that.

@Garyfallidis
Copy link
Contributor

@samuelstjean, @MrBago I am going to have a PR soon (but after the release) which will use some of these functions. I would suggest to clean up these functions after that. @samuelstjean thank you for the pep8 update. This was old code from the time were we were not forcing pep8. Thx!

Garyfallidis added a commit that referenced this pull request Nov 26, 2013
@Garyfallidis Garyfallidis merged commit 0025db7 into dipy:master Nov 26, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants