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 the determination of scale and rotation matrix from the CDij matrix #6573

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

ayshih
Copy link
Member

@ayshih ayshih commented Nov 7, 2022

  • Fixes a bug with the .rotation_matrix property where the PCij matrix was not determined correctly from the CDij matrix in the case of non-square pixels.
  • Fixes the .scale property to return reasonable values when a map has a CDij matrix and no CDELTi values. The returned pixel scales are the values that are needed so that each row of the inferred PCij matrix has unity norm.

@ayshih ayshih added the map Affects the map submodule label Nov 7, 2022
@ayshih ayshih changed the title Fixed the inference of scale and rotation matrix from the CDij matrix Fixed the determination of scale and rotation matrix from the CDij matrix alone Nov 7, 2022
@ayshih ayshih marked this pull request as ready for review November 7, 2022 14:07
@ayshih ayshih requested a review from a team as a code owner November 7, 2022 14:07
@ayshih ayshih changed the title Fixed the determination of scale and rotation matrix from the CDij matrix alone Fixed the determination of scale and rotation matrix from the CDij matrix Nov 7, 2022
@nabobalis nabobalis added this to the 4.1.0 milestone Nov 7, 2022
Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

Looks good, it's nice that we can now have feature parity with the CD matrix. My ability to fully comprehend the FITS spec was clearly not up to scratch when I first implemented this.

Comment on lines 1275 to 1276
If the CDij matrix is defined but no CDELTi values are explicitly defined,
effective CDELTi values are constructed from the CDij matrix.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to explain how here, because as you pointed out in #6570 there's not one singular "correct" way to do this. (Which is why it didn't used to do it)

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want this in this PR or another one?

Copy link
Member

Choose a reason for hiding this comment

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

This one preferably.

Copy link
Member

@dstansby dstansby Nov 9, 2022

Choose a reason for hiding this comment

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

+1 to documenting how .scale is calculated without CDELT metadata present in this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Elaborated

Copy link
Contributor

@nabobalis nabobalis left a comment

Choose a reason for hiding this comment

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

Blocking until Stuart is happy

@dstansby
Copy link
Member

dstansby commented Nov 9, 2022

The returned pixel scales are the values that are needed so that each row of the inferred PCij matrix has unity norm.

What's the reasoning for this choice? Does it make the resulting scale have some nice mathematica; properties?

@ayshih
Copy link
Member Author

ayshih commented Nov 10, 2022

The reason to make this choice (which is the optional constraint suggested in Paper I, equation 4) is so that if the rows of the CDij matrix are orthogonal, then the rows of the PCij matrix are orthonormal. That's your best shot at getting to a matrix that is a pure rotation (or reflection), if that is possible.

@nabobalis
Copy link
Contributor

Doc fail is due to another PR that I merged that broke it.

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

Successfully merging this pull request may close these issues.

4 participants