Fix make_fitswcs_transform error when handling header with no PC #142
Conversation
| xy_ans = np.array([120, 100]) | ||
| radec_deg_ans = (300.2308791294835, 22.691653517073615) | ||
|
|
||
| # TODO: Check with Nadia |
There was a problem hiding this comment.
@nden , I don't understand
- why
w()result can't agree for defaultrtol=1e-7 - why
w.inverse()would give me original XY + 1 and not XY
Am I using it wrong?
There was a problem hiding this comment.
GWCS by default uses 0-based pixel coordinates. CRPIX values are 1-based. So if you want to compare the inverse transform, it's going to return 0-based values.
There was a problem hiding this comment.
Though maybe I'm misunderstanding the problem?
There was a problem hiding this comment.
I am just generally confused... What I am seeing is this:
ra, deg = w(x, y)
but
w.inverse(ra, dec) = x + 1, y + 1
All I want to know is if this is the expected behavior.
There was a problem hiding this comment.
@jdavies-st is correct. When you are mixing FITS and gwcs you have to take care of 0- vs 1- based coordinates. The gwcs transform is 0-based. However, the values of crpix are 1-based. If you change the CRPIX values to be 0-based, i.e. subtract 1 from them , you will get agreement (you'll get better agreement in the forward direction too).
BTW, the header has incorrect FITS WCS to start with. It is not allowed to mix CD matrix and CDELT values and in this case the CDELT values are ignored and assumed to be 1.
I don't want to encourage changes and "fixes" to these functions as I think it's time to implement full conversion between the two objects.
There was a problem hiding this comment.
Ah, so I have to -1 from the keyword value, not inputs. Gotcha.
So, should I close this and wait for your "full conversion" then?
| @@ -7,15 +7,11 @@ | |||
| import functools | |||
| import numpy as np | |||
| from astropy.modeling import models as astmodels | |||
| from astropy.modeling.models import Mapping | |||
| from astropy.modeling import core, projections | |||
| from astropy.modeling import core, projections, is_separable | |||
There was a problem hiding this comment.
Missing import is a real bug found by my flake8 checker. If you want, I can put this in as separate PR. Though none of your existing tests caught it, so probably not important?
|
Just to clarify, the reason I don't use these functions is that I think that GWCS should not have to parse FITS headers but use the astropy.wcs parser. |
Fix #139. (Also threw in some PEP8 clean up for
utilsas bonus.)