Skip to content

Fix PC to CD matrix computation in HSTWCS#228

Open
mcara wants to merge 1 commit into
spacetelescope:mainfrom
mcara:fix-pc2cd
Open

Fix PC to CD matrix computation in HSTWCS#228
mcara wants to merge 1 commit into
spacetelescope:mainfrom
mcara:fix-pc2cd

Conversation

@mcara

@mcara mcara commented Aug 29, 2025

Copy link
Copy Markdown
Member

Closes #126

This PR fixes how CD matrix is computed from PC and CDELT.

Checklist for maintainers

  • added entry in CHANGELOG.rst within the relevant release section
  • updated or added relevant tests
  • updated relevant documentation
  • added relevant label(s)

@mcara
mcara requested a review from nden August 29, 2025 04:25
@mcara mcara self-assigned this Aug 29, 2025
@mcara mcara added the bug label Aug 29, 2025
@mcara

mcara commented Aug 29, 2025

Copy link
Copy Markdown
Member Author



def test_pc2cd():
e = np.exp(1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why not use np.e?

Comment thread stwcs/wcsutil/hstwcs.py
def pc2cd(self):
if self.wcs.has_pc():
self.wcs.cd = self.wcs.pc * self.wcs.cdelt[1]
self.wcs.cd = self.pixel_scale_matrix

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This will work for HST files because they don't have a PC matrix. However, the example in the test generates an incorrect file because it has now a CD, a PC, and CDELT all defined.

I'm not sure offhand what the correct action here is. I need to look into how this is used and whether it should be deleted.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

the test generates an incorrect file because it has now a CD, a PC, and CDELT all defined

@nden Which test?

@s-goldman

Copy link
Copy Markdown
Collaborator

@mcara any update on this?

@mcara

mcara commented Jan 11, 2026

Copy link
Copy Markdown
Member Author

well, no review, so I'll be bypassing protections

@mcara

mcara commented Jan 11, 2026

Copy link
Copy Markdown
Member Author

oops, I can't bypass. So, if you mind approving this, I can merge it.

@mcara

mcara commented Jan 11, 2026

Copy link
Copy Markdown
Member Author

Eh, I need to rethink this. This PR is correct, per se. It is not doing anything about headers and stuff - it just fixes a bad formula (or improves it). However, hstwcs.py imports pc2cd() from altwcs but then re-defines it. This is not good and it is probably indicative of some issue. It needs to be fixed either in this PR or a follow-up. I just need to understand what is the desired solution.

@mcara

mcara commented Jan 11, 2026

Copy link
Copy Markdown
Member Author

... hstwcs.py imports pc2cd() from altwcs but then re-defines it. This is not good and it is probably indicative of some issue. It needs to be fixed either in this PR or a follow-up. I just need to understand what is the desired solution.

After looking at this, there is no issue here: one pc2cd() is a member of HSTWCS class while the other is at the global level. Yes, it is a little confusing but not wrong.

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.

Why is PC matrix multiplied by cdelt1 to get CD matrix

3 participants