ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Add ability to import/export astropy-regions files in Drawing plugin #1003

Closed ejeschke closed 2 years ago

ejeschke commented 2 years ago
ejeschke commented 2 years ago

@pllim, review when you have a chance. I propose to merge this PR for release 3.4 and bump all the other PRs/issues forward for 4.0.

I think this makes Drawing a whole lot more interesting for some Ginga/astropy/ds9 users. Can draw in Ginga and load in ds9 or any astropy-regions application, or vice versa (only objects compatible with astropy-regions, but that's a fair number).

pllim commented 2 years ago

The GUI seems to work great when I drew some basic shapes, export, and re-import. I did not try to stress test it like drawing by WCS on one image and then reload on another one with a different WCS, or such.

ejeschke commented 2 years ago

You don't want to skip over the invalid ones? The way it is now, this will crash if any of them is invalid, right?

Converting from regions => ginga, the regions package will read the whole file before returning a Regions object, so if there are any errors in the user's file they will be handled by the regions package for importing. I have also added an optional logger parameter to each of the import and export functions so that any individual errors can be logged and skipped if a valid logger is passed.

ejeschke commented 2 years ago

@pllim, I think I addressed all of the major concerns posted in the review. In addition to a couple of round-trip tests, I even added a test for the case where a ginga object cannot be converted (no matching type in regions); it converts it to a text object with the error message if a logger is passed. Can you have another look to see if there are any remaining concerns?

ejeschke commented 2 years ago

Thanks, @pllim!