Conversation
|
If this is updating the orientation of the cutouts, how was this missed the first time? Was our comparison between old and new not sufficiently detailed to include orientation checks, along with pixel-level checks I am fairly certain were done? Or was the testing focused on the FITS output and not sufficient coverage of manual testing on the JPG output? |
|
@scfleming The orientation problem affects only JPEG (or other graphic formats). My guess is that the existing checks did not check the orientation on those images. Checking that the FITS cutouts are correct at the pixel level would not pick up this problem. |
Historically, the orientation of image outputs has never been flipped. The new and old version output the same thing, but they were both incorrect. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #180 +/- ##
==========================================
+ Coverage 95.92% 95.96% +0.04%
==========================================
Files 20 20
Lines 2232 2279 +47
==========================================
+ Hits 2141 2187 +46
- Misses 91 92 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
OK roger that, a good point to take stock of what we test as we develop, that's all. Glad we are getting this fixed with good tests, etc now. |
|
I wasn't fully aware of the convention for flipping images, so thank you @rlwastro for letting me know! I added some assertions to the tests that check that the image was flipped. |
|
|
||
|
|
||
| @pytest.mark.parametrize("colorize", [True, False]) | ||
| def test_fits_cutout_img_flip(test_images, center_coord, cutout_size, colorize): |
There was a problem hiding this comment.
Most of the changes in this file are stylistic. This is the new test!
| from pathlib import Path | ||
| from typing import List, Optional, Union, Tuple | ||
| import warnings | ||
| from abc import ABC, abstractmethod |
There was a problem hiding this comment.
There are a lot of style changes in this file as well. The relevant changes have to do with the flip_orientation parameter.
|
Do we embed any metadata into the png image cutouts? Like coordinates or wcs or anything like that? If not, we may want to consider that. It may help with verification of correct orientation in the future, or allow users to overlay onto other things. |
|
TIL you can add metadata to images! I love this idea. I added a few keywords that were pretty trivial, but I think something like a wcs (or gwcs in the case of ASDF) would require a little more work and may be outside the scope of this PR. This is something I will add to the backlog though. |
| meta.update( | ||
| { | ||
| "center_ra_deg": self._coordinates.ra.deg.item(), | ||
| "center_dec_deg": self._coordinates.dec.deg.item(), | ||
| "origin": "STScI/MAST", | ||
| "version": __version__, | ||
| } |
There was a problem hiding this comment.
should we also encode the pixel scale used in the cutout? Or cutout size or some info about the boundaries?
There was a problem hiding this comment.
Added! You can also access the dimensions through the width and height attributes of the Image object, but I figure we can make them more obvious.
Yeah, I embedded a standard FITS WCS for some png image cutouts with reasonable success. It was accurate enough to reasonably recover and overlay the image onto other FITS. The one caveat is the conversion to/from can be tricky since you have to convert every field to a string. gwcs might be trickier, but may also not be needed for a simple image cutout. |
I may be incorrect about this, but I thought the older PanSTARRS cutout service provided JPEG output with metadata/WCS embedded in them? I am fairly certain something at MAST had/has this capability, and Rick might know about it....just FYI, I agree adding WCS is a great idea and could be very useful especially with some of the stuff Cobalt team is doing (they might also have capability of exporting JPG with WCS info embedded for overlaying on Aladin), but as a separate PR. |
|
@scfleming Scott is correct, the current fitscut service does add the WCS to JPEG images in the JPEG comments section. This was originally added because it was supported by Aladin, which can display a JPEG with an embedded WCS. It turns out to be pretty easy to access from within Python using PIL. |
|
when we add the WCS in the output, you can check with Brett on how they are handling the aladin overlay with Roman gwcs, e.g., I think they are using an approximation of gwcs to handle what FITS supports, which is probably sufficient for use within Aladin especially over a small area, but could to check in with them what they've learned since they are also working on this for exporting JPG from Jdaviz as overlays on Aladin. We'd want astrocut JPG output to have same precision and implementation as what's being done for Jdaviz I think. |
When creating
PIL.Imageobjects or writing cutouts to image formats, vertically flip the image to match the intended orientation of astronomical data.Added a
flip_orientationparameter toget_image_cutoutsandwrite_as_imgto allow users to opt in or out. The default is True, but I wanted to keep the legacy option of the "unflipped" version.