Skip to content

Fixed Gaussian2D model when a cov_matrix is input#2199

Merged
embray merged 8 commits into
astropy:masterfrom
larrybradley:2dgauss-covmatrix
Apr 2, 2014
Merged

Fixed Gaussian2D model when a cov_matrix is input#2199
embray merged 8 commits into
astropy:masterfrom
larrybradley:2dgauss-covmatrix

Conversation

@larrybradley

Copy link
Copy Markdown
Member

This PR fixes the rotation direction of the Gaussian2D model when a cov_matrix is input. theta is calculated from cov_matrix as the angle of the major-axis eigenvector using np.arctan2(), which returns a counterclockwise angle relative to the positive x axis. However, the theta used by Gaussian2D is defined in a clockwise direction. This PR corrects the rotation direction.

@astrofrog astrofrog added this to the v0.3.2 milestone Mar 16, 2014
@astrofrog

Copy link
Copy Markdown
Member

@cdeil, since you looked into the rotation issue, do you agree with this?

@larrybradley

Copy link
Copy Markdown
Member Author

Here's one way you can show that the current rotation is wrong:
http://nbviewer.ipython.org/gist/larrybradley/9808977

@astrofrog

Copy link
Copy Markdown
Member

Ok, that looks wrong :) Could you add a regression test? Maybe you could do it with a simple low-res 5-5 array?

larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 27, 2014
larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 27, 2014
@larrybradley

Copy link
Copy Markdown
Member Author

@astrofrog I've added the test, updated the change log, and rebased.

larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 27, 2014
larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 27, 2014
larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 27, 2014
larrybradley added a commit to larrybradley/astropy that referenced this pull request Mar 27, 2014
@cdeil

cdeil commented Mar 28, 2014

Copy link
Copy Markdown
Member

I agree this fix is correct to be consistent with the definition in Gaussian2D

The rotation angle increases clockwise.

It took me a while to convince myself this fix is correct. It could be useful to add in the notes section explaining the relation of the covariance matrix to the formulas given, namely that cov_matrix = [[a, b], [b, c]] and maybe even that positive b corresponds to a theta in the range 0 to 90 deg. (if that is correct).

But ... :-)

Should we re-consider the choice of rotation angle direction?
The other tools I use (sherpa gauss2d and SExtractor both use a rotation angle that increases counter-clockwise.
I think having the same convention as other common tools would be enough motivation to change.
@larrybradley @astrofrog What do you think? Are there other important tools that use rotation angles that increase clockwise?

@nden

nden commented Mar 28, 2014

Copy link
Copy Markdown
Contributor

I am in favor of consistently using counter-clockwise rotation (including RotateByAngle which does a clockwise rotation now) as this seems more intuitive to me. 👍 to changing all of them to counter-clockwise.

@larrybradley

Copy link
Copy Markdown
Member Author

I am also in favor of consistently using counter-clockwise rotation. I never liked that it was clockwise. If @astrofrog concurs, then I'll change the rotation.

@larrybradley

Copy link
Copy Markdown
Member Author

While we are discussing this, I'd like to bring up one other point. Currently there is a degeneracy in the model such that one can swap x_stddev and y_stddev and add 90 degrees to theta and get the same exact model. This could lead to some confusion when fitting the model. Does it make sense to perhaps change the inputs to something like major/minor axis stddev and then have theta represent the angle of the major axis (w.r.t to +x axis for example). There is still the 180 degree degeneracy, but nothing can be done about that. Thoughts?

@astrofrog

Copy link
Copy Markdown
Member

Counter-clockwise makes sense!

@larrybradley

Copy link
Copy Markdown
Member Author

OK, I've changed it to counterclockwise rotation. This should be ready to merge.

I'll update the MatrixRotation2D model next.

@cdeil

cdeil commented Apr 1, 2014

Copy link
Copy Markdown
Member

👍 to merge.

embray added a commit that referenced this pull request Apr 2, 2014
Fixed Gaussian2D model when a cov_matrix is input
@embray embray merged commit 19eb965 into astropy:master Apr 2, 2014
@larrybradley larrybradley deleted the 2dgauss-covmatrix branch April 2, 2014 18:54
@embray

embray commented Apr 17, 2014

Copy link
Copy Markdown
Member

Any clarifications on the addition of this change to the 0.3.2 milestone? The changelog entry is under 0.4. Also as this constitutes a major change in behavior I would probably leave in as 0.4 anyway.

@embray embray modified the milestones: v0.4.0, v0.3.2 Apr 17, 2014
ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014
ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014
ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014
ktchrn pushed a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014
@embray embray mentioned this pull request Nov 7, 2014
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.

5 participants