Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement saving regions to ECSV via astropy Table #551

Open
rosteen opened this issue May 15, 2024 · 8 comments
Open

Implement saving regions to ECSV via astropy Table #551

rosteen opened this issue May 15, 2024 · 8 comments

Comments

@rosteen
Copy link

rosteen commented May 15, 2024

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 spacetelescope/jdaviz#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
Copy link
Member

pllim commented May 15, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link

Chiming in to say the opposite of @larrybradley. 🥲

@keflavich
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

larrybradley commented May 17, 2024

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: astropy/astropy-project#375

@pllim
Copy link
Member

pllim commented May 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants