astropy / regions

Astropy package for region handling
https://astropy-regions.readthedocs.io
BSD 3-Clause "New" or "Revised" License
61 stars 55 forks source link

Pixel Representations #66

Open sargas opened 8 years ago

sargas commented 8 years ago

I'm looking at porting my ply-based parsing from https://github.com/sargas/pyregion/blob/regions-submodule/pyregion/regions/_ply_helper.py , which I think is more flexible (would fix #65 , for instance, and I have many tests)

One issue I notice is that regions can be defined in various frames - FK5, galactic, etc - but pixel regions lose all information of how they are defined. If I write in a DS9 file:

circle 50 50 5

This is referring to some coordinate system defined by a FITS file header (names of these include image, physical, detector, pane, gap, amplifier, ccd, primary, etc). Do subclasses of PixelRegion include this information?

If not, it seems like someone could easily get confused trying to display a region defined by a FITS file with non-trivial LTM/LTV values and getting the region at a different place then in CIAO or DS9.

cdeil commented 8 years ago

I'm looking at porting my ply-based parsing from https://github.com/sargas/pyregion/blob/regions-submodule/pyregion/regions/_ply_helper.py , which I think is more flexible (would fix #65 , for instance, and I have many tests)

@sargas - Very interesting! Could you please make a separate issue to discuss the possibility of switching to your ply-based parser? If you have some timing, could you include it there and mention if it's faster or slower than the current one in pyregion or regions? (see https://github.com/astropy/pyregion/issues/48)

Do subclasses of PixelRegion include this information?

There's meta, but I'm not sure this is the info you're talking about here. (see http://astropy-regions.readthedocs.io/en/latest/getting_started.html#ds9). @keflavich - Do you know? @sargas - Could you give a small example and show where the info you mean is in pyregion, and then we can discuss where it should go in regions.

I think the data model for regions isn't fully thought out. We could either replicate what ds9 / pyregion do, or do something simpler and just have pixel regions as now, or use STC-S (see #21). Suggestions welcome!

sargas commented 8 years ago

An example of a header where this matters is sample_fits01.header from pyregion.

Comparing regions and pyregions:

import regions, pyregion
from astropy.io.fits import Header
print(pyregion.parse('circle(4,2,5)').as_imagecoord(Header.fromtextfile('sample_fits01.header'))[0].coord_list)
print(regions.ds9_string_to_objects('physical;circle(4,2,5)')[0])

The first returns [-3300.0, -3267.0, 5.0], since the LTV/LTM keywords define a physical coordinate system offset and scaled from the image coordinates. The second prints a CirclePixelRegion centered at (4,2), but is otherwise stuck due to a lack of a to_pixel method to convert to image coordinates.

(as an aside, ds9/ciao region files use physical coordinates unless otherwise specified, but regions wouldn't parse unless it was explicit)

pyregion handles this by having each shape keep track of what coordinate system they are (in a .coord_format attribute). pyregion uses this here with if/elif statements for sky and physical coordinates, although the version in my PR is more readable.

sargas commented 8 years ago

I kinda think the best approach would be to view pixel coordinates (and shapes using them) as being defined by some header file in some way.

I started this approach by hijacking SkyCoord to represent pixel coordinates with custom frames here, with examples here. Image<->Physical frame transformations could be added based on pyregion's physical coordinate code

This API extends to making a simple API for regions, where regions or shapes can be transformed simply as:

region_object.transform_to(frame_object)

where frame_object can be some sky coordinate system (e.g., ICRS) or pixel-based (i.e., physical, image, amplifier, etc).

This is a different approach then what regions is already using though, with separate SkyRegion and PixelRegion objects with exclusive functionality.

keflavich commented 8 years ago

I think we haven't gotten far enough in implementing astropy/regions to get the pixel frame set up correctly. This is closely related to the pixel-based coordinate systems we were discussing with the LSST folks; we may need to implement the original-pixel/cropped coordinate systems first. I'm not really sure how this has gone or where it's going. It looks like @sargas should be included in the discussion... @astrofrog, do you know where the discussion of offset pixel coordinates is happening?

cdeil commented 8 years ago

@sargas Thanks for the infos and links.

@astrofrog @eteq Would this astropy.regions package be the appropriate place to introduce and develop such pixel coordinate frames? Or is this being done somewhere else (in an APE?) or not clear if it should go in Astropy core at all?

astrofrog commented 8 years ago

I don't think pixel coordinate frames are being developed anywhere else, so I don't see the harm in experimenting here and deciding later if it belongs in the core?