astropy / regions

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

Implement saving regions to ECSV via astropy Table #551

Open rosteen opened 2 months ago

rosteen commented 2 months ago

I recently implemented IO to/from ECSV files via astropy.table in specutils, and coded up a quick minimal implementation to save out the spatial regions relevant to Jdaviz in https://github.com/spacetelescope/jdaviz/pull/2874 to provide export parity with spectral regions. I spoke to @larrybradley offline about upstreaming that code to here, and he pointed out that it would need to handle all the metadata available in regions, the other regions that we don't use in JDaviz, and implement a reader/parser as well. This issue is mostly to register my intent to do that at some point, and to provide a public place for any further discussion.

pllim commented 2 months ago

I voiced concern with the implementation in Jdaviz only because I worry that Jdaviz is creating a new format with no buy-in from regions (or Astropy). If people disagree with the Jdaviz format in the future, it would be more technical debt because then we would have to refactor, deprecate, etc.

My proposal is to have partial implementation here, throw NotImplementedError for things Jdaviz does not need yet, and open follow-up issues.

astrofrog commented 2 months ago

It might make sense to try and implement or at least scope out support for all regions from the start, to make sure the proposed format will actually work with all region types (in particular polygons)

larrybradley commented 2 months ago

If we're going to create a new regions file format, I wonder if asdf is a better way to go for serializing objects.

bmorris3 commented 2 months ago

Chiming in to say the opposite of @larrybradley. 🥲

keflavich commented 2 months ago

I don't think we're proposing a new regions file format? It should be possible, and even straightforward, to directly write all data/metadata associated with regions in a tabular format. I think it will be inefficient to use .ecsv, since it will likely be necessary to write out a lot of redundant information (.reg and .crtf formats allow unspecified information to be filled in implicitly), but it shouldn't be too hard to implement a reasonable ecsv reader.

That said, if we're using astropy tables, it shouldn't matter what format we're writing to if we're using astropy's unified table writing/reading scheme; it can write to FITS, ecsv, or whatever. Probably table can write to asdf too?

astrofrog commented 2 months ago

What if we thought about this as a new mixin column type which would be a column of regions in a table? (So then format is secondary as @keflavich says)

larrybradley commented 2 months ago

A new mixin column type is an interesting idea. I'm not sure how much work that would require.

FYI, I just remembered that @perrygreenfield proposed for astropy funding that included support for ASDF serialization of region (and photutils aperture) objects. It was approved 2 weeks ago: https://github.com/astropy/astropy-project/pull/375

pllim commented 2 months ago

As @keflavich pointed out above, metadata is already supported by other formats, so what value is ECSV adding to the mix?