Fix Off-By-One error in column and row coordinates

[barentsen]

Over in #7, @jcsmithhere correctly pointed out that tess-cloud currently interprets column and row coordinates in the "0-based indexing" scheme. This is inconsistent with the convention for the TESS mission, which is to use 1-based indexing for pixels. As a result, cutouts produced by tess-cloud are consistently off by one pixel in both the column and the row direction.

This PR fixes the issue and adds a unit test which compares the fluxes returned by tess-cloud with those obtained via TessCut, which acts as an effective test to verify that column/row indexing is consistent with the convention used by TessCut.

Below I post a before/after example. The example demonstrates how I reproduced the issue, and that the PR fixes the example shown.

Behavior before this PR

Behavior with this PR merged

[jcsmithhere]

I tried your fix and I believe there is a bug. Previously, if I generate a new TPF using tess-cloud, I get an error of about -1 pixel. But now using your fix, column is off by about 10 pixels and row by about 600 pixels. See screenshot comparing measured and predicted astrometry using your fix.

[jcsmithhere]

I tried using "git reset" to reverse your merges back to July 7th and I get good agreement using the exact same method as above. So your recent changes do appear to be introducing the large error.

[barentsen]

Oh dear. Thanks for reporting this. I did a few extra spot checks with tess_cloud.cutout() and am unable to reproduce the problem that way (see example below), but I'll try writing a proper unit test for moving cutouts next...

[jcsmithhere]

Hi @barentsen, I discovered what changed. Previously, in the tess-cloud generated tpf the fields tpf.row and tpf.column were set to zero. Now they appear to be the corner row and column on the first cadence. So what is happening is when I add in the CORNER_ROW and CORNER_COLUMN I am double adding the reference pixel.

If tpf.row and tpf.column are going to be the corner row and column on the first cadence then CORNER_ROW and CORNER_COLUMN should be relative to tpf.row and tpf.column.

I have related question. There is a namespace conflict. I notice that tess-cloud has its own TargetPixelFile class. This supersedes the TargetPixelFile class in LightKurve and yet doesn't inherent it. Why do you do this? My MovingTargetPixelFile class inherents the TessTargetPixelFile class in LightKurve, not tess-cloud. Now I'm not sure which I should inherent.

[barentsen]

Previously, in the tess-cloud generated tpf the fields tpf.row and tpf.column were set to zero. Now they appear to be the corner row and column on the first cadence.

Ahhh, yes, that is correct! This change was made to enable cutouts of non-moving objects to be plotted correctly in Lightkurve, because e.g. tpf.plot() depends on tpf.row and tpf.column to label the X/Y axes.

If tpf.row and tpf.column are going to be the corner row and column on the first cadence then CORNER_ROW and CORNER_COLUMN should be relative to tpf.row and tpf.column.

This of course depends on how we want to define CORNER_ROW and CORNER_COLUMN. I've imagined them as absolute coordinates in the standard CCD reference frame. In fact I think it would make sense for tpf.row and tpf.column to return the values of CORNER_ROW and CORNER_COLUMN in the case of a moving TPF, because a single value is misleading, if that makes sense?

I notice that tess-cloud has its own TargetPixelFile class. This supersedes the TargetPixelFile class in LightKurve and yet doesn't inherent it.

The lightkurve.TargetPixelFile class is essentially a wrapper around a FITS HDUList object (stored in tpf.hdu). In contrast, the tess_cloud.TargetPixelFile class attempts to be a more generic model for a target pixel file that does not depend on an existing FITS file. I can eventually see them merge, but you are absolutely correct that they are not compatible at this time. The tess_cloud.TargetPixelFile class is really only useful for creating a new FITS file right now, so I recommend using lightkurve.TargetPixelFile for all intents and purposes.

[jcsmithhere]

If tpf.row and tpf.column are going to be the corner row and column on the first cadence then CORNER_ROW and CORNER_COLUMN should be relative to tpf.row and tpf.column.

This of course depends on how we want to define CORNER_ROW and CORNER_COLUMN. I've imagined them as absolute coordinates in the standard CCD reference frame. In fact I think it would make sense for tpf.row and tpf.column to return the values of CORNER_ROW and CORNER_COLUMN in the case of a moving TPF, because a single value is misleading, if that makes sense?

I think we need to change something. Having them be in absolute coordinates makes the most universal sense. I agree that tpf.row and tpf.column simply returning the values of CORNER_ROW and CORNER_COLUMN is appropriate. My mtpf.animate code will need to be updated to account for this, but that can be done.