Remove redundant SCAMP distorition parameters when SIP is also present by lpsinger · Pull Request #1278 · astropy/astropy

lpsinger · 2013-07-23T07:18:35Z

Fixes #299.

mdboom · 2013-07-23T13:14:03Z

Thanks for doing this -- it was hard for me to prioritize work on #299, since we don't use SCAMP files here.

So when SCAMP files also have SIP the SIP is intended to supercede the PVi_ma records? Is that behavior defined somewhere? If so, I think this is reasonable, but I wouldn't necessarily say this fixes #299, since this doesn't mean we actually support SCAMP's use of PVi_ma. That doesn't mean this present PR isn't worth having in the interim, however.

mdboom · 2013-07-23T13:23:45Z

This change should not be made. If parsing of one of the header cards fails and the loop bails early, we still want access to the other ones, so we need to update npv as we go.

Then we need to set (*npv) = i + 1; inside the loop and (*npv) = i; outside it at the end.

nden · 2013-07-23T13:53:08Z

It seems like this PR removes PV keywords any time SIP is present. Isn't this correct ONLY for projections which don't use the PV keywords, like TAN. I am just trying to understand the changes here. If so, I would think this kind of fix belongs to an application software, not the library. Or at least _fix_scamp should not be called always. (I'm late to this, I may be misunderstanding the problem.)

I am not familiar with the SCAMP distortion model but I wonder if it's possible to convert it to the SIP convention (similar to how HST converted the IDCTAB distortion to SIP)?

mdboom · 2013-07-23T14:02:11Z

Very good point, @nden about overzealously removing PV keywords. Is there any other metadata we can use to detect that this is a SCAMP file?

I believe it is possible to convert the SCAMP distortion model to SIP, and this PR basically assumes that has already been done. I think that was the original proposal to fix #299, but it's more complex than this PR, of course.

lpsinger · 2013-07-23T15:56:43Z

So when SCAMP files also have SIP the SIP is intended to supercede the PVi_ma records? Is that behavior defined somewhere?

This behavior is suggested in the second bullet point in Section 7 of http://web.ipac.caltech.edu/staff/shupe/reprints/SIP_to_PV_SPIE2012.pdf.

If so, I think this is reasonable, but I wouldn't necessarily say this fixes #299, since this doesn't mean we actually support SCAMP's use of PVi_ma.

You are right. We also should convert the SCAMP distortion parameters to SIP if the SCAMP parameters are present but not SIP. I'd just need an example of an output file of SCAMP that hasn't had the SIP distortion parameters added like PTF does.

lpsinger · 2013-07-23T15:57:37Z

I am not familiar with the SCAMP distortion model but I wonder if it's possible to convert it to the SIP convention (similar to how HST converted the IDCTAB distortion to SIP)?

Yes, @nden, see http://web.ipac.caltech.edu/staff/shupe/reprints/SIP_to_PV_SPIE2012.pdf.

lpsinger · 2013-07-23T16:00:08Z

Is there any other metadata we can use to detect that this is a SCAMP file?

Possibly... SCAMP sets the comments for the PVi_m cards to the string Projection distortion parameter. As long as that comment would not make sense for other uses of PVi_m, we could use that.

mdboom · 2013-07-23T16:55:05Z

This is no longer incorrect, but it's still redundant. And the end of the loop, i is incremented, it is no longer less than size so it terminates with a value of what was i + 1 at the bottom of the loop.

@mdboom, that's a good point.

What do you think of having this function do all of the type marshaling before populating the WCS data structures, so that the data structure is updated if and only if the function has progressed past the point which any Python exceptions could possibly be raised?

That seems like a good idea, and definitely better in terms of "transaction safety" than what we have now.

mdboom · 2013-07-23T16:56:55Z

I'd prefer to detect the SCAMP file using something other than a comment. If that's the best we can do, so be it, but comments shouldn't be considered meaningful, in general.

stargaser · 2013-07-23T17:52:39Z

SCAMP always makes a fourth-order polynomial with no radial terms. I think that would be the best fingerprint. If SCAMP were changed to do something different, it would change which terms appear...I guess that could be handled later if it actually happens.

I don't know of any other signature for the presence of the PV distortion keywords. The proposed "TPV" convention would change the CTYPEs to e.g. RA---TPV instead of RA---TAN, but this isn't the current behavior of SCAMP as far as I know.

astrofrog · 2013-09-20T19:56:53Z

I think this needs to be rebased.

@lpsinger @mdboom - what is the status of this otherwise?

astrofrog · 2013-10-13T09:52:46Z

@lpsinger @mdboom - what is the status of this?

mdboom · 2013-10-14T15:11:17Z

It needs a rebase, but I think it's fine otherwise. Not ideal, since it uses a strange heuristic to detect incorrect (or obsolete) use of the FITS WCS spec, but as a real world solution, I think it's the best we've got.

lpsinger · 2013-10-14T18:19:52Z

Ideally, we should implement this condition:

SCAMP always makes a fourth-order polynomial with no radial terms. I think that would be the best fingerprint. If SCAMP were changed to do something different, it would change which terms appear...I guess that could be handled later if it actually happens.

mdboom · 2013-10-14T18:40:42Z

@lpsinger: I agree. Are you able to implement that, or shall I take a crack at it?

lpsinger · 2013-10-14T18:48:50Z

Feel free to take a crack at it if you want. I could get to it by late this week.

Fixes astropy#299.

The number of pv cards was being updated inside a loop. If there were 0 pv cards, then the loop body would never be executed, hence the number of pv values would not be updated. The loop counter itself, upon the end of the loop, is the number of pv cards.

The WCS.wcs.get_pv() accessor seems to correctly return an empty array if there are no PVi_Ma records.

Since the fix() method now repairs duplicate SCAMP/SIP distortion parameters, the validation now only fails if the WCS has not been fixed.

...so that it has a correct value even if the loop ends early because an error occurs.

Either the entire input is converted from Python types and updated successsfully, or no change to the internal data structures occur. Also, should be a little bit more lightweight due to using PySequence_Fast to eliminate need for bounds checking in loop.

SCAMP inserts PV parameters with no radial terms, so if there are any PV terms with indices that are used for storing radial parameters, we can conclude that the PV parameters do not come from SCAMP. See <http://web.ipac.caltech.edu/staff/shupe/reprints/SIP_to_PV_SPIE2012.pdf>.

lpsinger · 2013-10-17T22:30:45Z

I have both implemented the check for radial terms suggested by @stargaser and rebased the patches. I think that it is ready to go.

eteq · 2013-10-18T20:48:52Z

Those failures don't look real to me, but they keep happening even when I restart... I think there might be some sort of network issue on travis' side? So is this ready to go, @mdboom, assuming that's the case?

astrofrog · 2013-10-18T21:18:09Z

@eteq - yeah, Travis is broken currently, I guess we just need to be patient.

astrofrog · 2013-10-21T15:41:13Z

@mdboom - I restarted the tests. Is this good to merge once the tests pass?

mdboom · 2013-10-21T15:46:14Z

I think <= here?

mdboom · 2013-10-21T15:50:15Z

Other than my one minor comment, this looks good once Travis is passing.

astrofrog · 2013-10-21T15:58:02Z

@lpsinger - once you've addressed @mdboom's comment above, I'll merge this so we can get it in 0.3.

lpsinger · 2013-10-21T16:09:29Z

@lpsinger - once you've addressed @mdboom's comment above, I'll merge this so we can get it in 0.3.

Done.

lpsinger · 2013-10-21T16:34:23Z

All but one of the Travis tests passed; the last one timed out. I just restarted that job.

lpsinger · 2013-10-21T17:17:51Z

All tests passed.

astrofrog · 2013-10-21T17:25:47Z

@lpsinger - thanks!

Remove redundant SCAMP distorition parameters when SIP is also present

mdboom reviewed Jul 23, 2013
View reviewed changes

mdboom mentioned this pull request Jul 23, 2013

Wcs/scamp sip distortion lpsinger/astropy#1

Merged

mdboom reviewed Jul 23, 2013
View reviewed changes

lpsinger mentioned this pull request Jul 23, 2013

Add iterative wcs.all_world2pix #1281

Merged

lpsinger added 9 commits October 17, 2013 14:03

Remove redundant SCAMP distorition parameters when SIP is also present

2325a09

Fixes astropy#299.

Remove assertion if no PVi_ma records are present

3ff65e2

The WCS.wcs.get_pv() accessor seems to correctly return an empty array if there are no PVi_Ma records.

Test case has to set fix=False

bf7e6a5

Since the fix() method now repairs duplicate SCAMP/SIP distortion parameters, the validation now only fails if the WCS has not been fixed.

Update list of warnings that are expected to be raised

321c464

Add minimal unit test after removing duplicate PVi_m parameters

5ea405f

Set number of PVi_m cards inside and outside of loop

de9403a

...so that it has a correct value even if the loop ends early because an error occurs.

Explain in comment why we have to set fix=False

9516e5c

Wrap long line to 80 characters

88121ca

mdboom and others added 3 commits October 17, 2013 14:05

Fix how the NULL pointer check for ps and pv cards is done.

737d465

eteq mentioned this pull request Oct 18, 2013

astropy.wcs: presence of keywords like "PV1_5" causes error #299

Closed

mdboom reviewed Oct 21, 2013
View reviewed changes

Comment thread astropy/wcs/src/pyutil.c Outdated

mdboom Oct 21, 2013

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

I think <= here?

Correct comparison for allocating new storage for PV cards

a9d2685

astrofrog added a commit that referenced this pull request Oct 21, 2013

Merge pull request #1278 from lpsinger/wcs_scamp_sip_distortion

cc2a9ee

Remove redundant SCAMP distorition parameters when SIP is also present

astrofrog merged commit cc2a9ee into astropy:master Oct 21, 2013

lpsinger deleted the wcs_scamp_sip_distortion branch October 21, 2013 17:34

embray removed the Ready-for-final-review label Jan 13, 2015

astrofrog mentioned this pull request Mar 1, 2015

Better error for SCAMP WCS or support for TPV #3559

Closed

navii98 mentioned this pull request Jan 7, 2023

Presence of SIP keywords leads to ignored PV keywords. #14255

Closed

Conversation

lpsinger commented Jul 23, 2013

Uh oh!

mdboom commented Jul 23, 2013

Uh oh!

mdboom Jul 23, 2013

Choose a reason for hiding this comment

Uh oh!

lpsinger Jul 23, 2013

Choose a reason for hiding this comment

Uh oh!

nden commented Jul 23, 2013

Uh oh!

mdboom commented Jul 23, 2013

Uh oh!

lpsinger commented Jul 23, 2013

Uh oh!

lpsinger commented Jul 23, 2013

Uh oh!

lpsinger commented Jul 23, 2013

Uh oh!

mdboom Jul 23, 2013

Choose a reason for hiding this comment

Uh oh!

lpsinger Jul 23, 2013

Choose a reason for hiding this comment

Uh oh!

mdboom Jul 23, 2013

Choose a reason for hiding this comment

Uh oh!

mdboom commented Jul 23, 2013

Uh oh!

stargaser commented Jul 23, 2013

Uh oh!

astrofrog commented Sep 20, 2013

Uh oh!

astrofrog commented Oct 13, 2013

Uh oh!

mdboom commented Oct 14, 2013

Uh oh!

lpsinger commented Oct 14, 2013

Uh oh!

mdboom commented Oct 14, 2013

Uh oh!

lpsinger commented Oct 14, 2013

Uh oh!

lpsinger commented Oct 17, 2013

Uh oh!

eteq commented Oct 18, 2013

Uh oh!

astrofrog commented Oct 18, 2013

Uh oh!

astrofrog commented Oct 21, 2013

Uh oh!

mdboom Oct 21, 2013

Choose a reason for hiding this comment

Uh oh!

mdboom commented Oct 21, 2013

Uh oh!

astrofrog commented Oct 21, 2013

Uh oh!

lpsinger commented Oct 21, 2013

Uh oh!

lpsinger commented Oct 21, 2013

Uh oh!

lpsinger commented Oct 21, 2013

Uh oh!

astrofrog commented Oct 21, 2013

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants