DM-54556: Add VisitImage.to_legacy and supporting code. by TallJimbo · Pull Request #44 · lsst/images

TallJimbo · 2026-05-20T23:09:34Z

Checklist

ran Jenkins
added a release note for user-visible changes to doc/changes

codecov · 2026-05-20T23:10:35Z

Codecov Report

❌ Patch coverage is 23.82979% with 179 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.30%. Comparing base (fa2e7ef) to head (58cfa82).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
python/lsst/images/_visit_image.py	15.73%	75 Missing ⚠️
tests/test_visit_image.py	2.38%	41 Missing ⚠️
python/lsst/images/fields/_base.py	30.76%	18 Missing ⚠️
tests/test_image.py	0.00%	10 Missing ⚠️
tests/test_mask.py	0.00%	10 Missing ⚠️
python/lsst/images/_observation_summary_stats.py	27.27%	8 Missing ⚠️
python/lsst/images/_image.py	50.00%	4 Missing ⚠️
tests/test_masked_image.py	62.50%	3 Missing ⚠️
python/lsst/images/fits/_common.py	77.77%	2 Missing ⚠️
python/lsst/images/psfs/_piff.py	60.00%	2 Missing ⚠️
... and 5 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #44      +/-   ##
==========================================
- Coverage   74.15%   73.30%   -0.86%     
==========================================
  Files          90       89       -1     
  Lines       10556    10718     +162     
==========================================
+ Hits         7828     7857      +29     
- Misses       2728     2861     +133

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

timj

Looks good.

timj · 2026-05-21T22:47:46Z


+    @property
+    def is_constant(self) -> bool:
+        return (self._data == self._data[0, 0]).all()


Maybe add a note that exact floating point comparison here is deliberate.

timj · 2026-05-21T22:52:44Z

+            pass
+        else:
+            # Image already has calibrated pixel units; return a constant.
+            # TODO[DM-54556]: make sure this shouldn't be 1/factor.


I assume this ticket is blocking us using this code for DP2 and prompt so will be fixed soon?

Aha, no! That is this very ticket, and I forgot to go back and deal with it before sending out for review.

It actually can't come up at all right now; we'll always have factor == 1.0 when converting from afw. So it's kind of annoying that the extra generality of lsst.images creates this hypothetical problem. But it's fixed and tested now, and that extra testing revealed another problem due to an afw limitation (in field_to_legacy_photo_calib) that I've now fixed (and then squashed; sorry).

Oops. I didn't notice it was this ticket 😄

timj · 2026-05-21T22:56:54Z

+        # legacy metadata conponent, to make sure we have everything.
+        if isinstance(self._opaque_metadata, FitsOpaqueMetadata):
+            result_info.getMetadata().update(self._opaque_metadata.headers[ExtensionKey()])
+        result_info.getMetadata().update(self.metadata)


In some sense it might be preferable to namespace the modern non-FITS metadata so that it can't overwrite a pre-existing FITS header.

So something like:

result_info.getMetadata().update({f"LSST IMAGE {k.upper()}": v for k, v in self.metadata.items()})

and then from_legacy can reverse that.

Good idea! I've taken it a step further and mapped each item it to a pair of LSST IMAGES KEY {n} and LSST IMAGES VALUE {n} cards, so we can preserve case, too.

Testing this reminded me that I hadn't been thinking of round-tripping VisitImage through the Exposure file format as a goal, because I don't think we necessarily need it for the Prompt or DP2 transition. That revealed a few new issues related to unit conversion (we have to fight Astropy to let us read and write non-standard BUNIT values) and data IDs (since rewriting should drop the LSST BUTLER DATAID keys, but we want those on read). Please take a look at the two new commits.

TallJimbo · 2026-05-22T14:06:24Z

+            pass
+        else:
+            # Image already has calibrated pixel units; return a constant.
+            # TODO[DM-54556]: make sure this shouldn't be 1/factor.


Aha, no! That is this very ticket, and I forgot to go back and deal with it before sending out for review.

It actually can't come up at all right now; we'll always have factor == 1.0 when converting from afw. So it's kind of annoying that the extra generality of lsst.images creates this hypothetical problem. But it's fixed and tested now, and that extra testing revealed another problem due to an afw limitation (in field_to_legacy_photo_calib) that I've now fixed (and then squashed; sorry).

TallJimbo · 2026-05-22T14:19:58Z

+        # legacy metadata conponent, to make sure we have everything.
+        if isinstance(self._opaque_metadata, FitsOpaqueMetadata):
+            result_info.getMetadata().update(self._opaque_metadata.headers[ExtensionKey()])
+        result_info.getMetadata().update(self.metadata)


Good idea! I've taken it a step further and mapped each item it to a pair of LSST IMAGES KEY {n} and LSST IMAGES VALUE {n} cards, so we can preserve case, too.

Testing this reminded me that I hadn't been thinking of round-tripping VisitImage through the Exposure file format as a goal, because I don't think we necessarily need it for the Prompt or DP2 transition. That revealed a few new issues related to unit conversion (we have to fight Astropy to let us read and write non-standard BUNIT values) and data IDs (since rewriting should drop the LSST BUTLER DATAID keys, but we want those on read). Please take a look at the two new commits.

TallJimbo · 2026-05-22T16:40:10Z

-            visit = _extract_or_check_header("LSST BUTLER DATAID VISIT", visit, primary_header, None, int)
+            visit = _extract_or_check_header(
+                "LSST BUTLER DATAID VISIT", visit, primary_header, obs_info.visit_id, int
+            )


@timj I had to change this to get the visit ID from the ObservationInfo when LSST BUTLER DATAID VISIT isn't (as is the case in the tests when we write a VisitImage as an lsst.afw.image.Exposure, at least when that uses a data ID that doesn't have "visit" in it. I wanted to make sure this is okay, since it seemed like you might have had a reason for not falling back to it before.

Oh. Right. I think you have to use obs_info.exposure_id because the ObservationInfo visit_id is not what you actually want and now we know that visi ID and exposure ID are meant to be the same.

timj

The new metadata header writing looks good to me.

timj · 2026-05-22T17:14:35Z

        strip_butler_cards(primary_header)
+        metadata: dict[str, Any] = {}
+        for n in itertools.count():
+            if (key := header.pop(f"LSST IMAGES KEY {n + 1}", None)) is None:


Technically a Astropy FITS Header can have a value of None but if you've written an undefined value into the header card you probably have other issues.

Well, it's not a such a crazy thing to do if you consider the fact that we're trying to round-trip a dict that may well have a None value. I'll switch to ... as the sentinal value.

Since these aren't typed upstream, this does very little for static analysis, but Sphinx uses these types, too, and it's just a better pattern to establish.

None of this test would run without afw.

Since the concrete field types that can support to_legacy already do, this just moves a type-analysis error into a runtime error (which is what we want). That in turn helps us add a PhotoCalib converter to the base class.

'band' is a new required attribute. 'ID' is just stuffed into the metadata for now - we'd like to retire it someday, but as long as we need to be able to roundtrip a legacy VisitImage we'll need to carry it around.

This provides testing for code in obs_base.

All code using this module was removed on DM-54976.

This will inevitably drop background information and photometric scalings attached to already-calibrated images (because there's nowhere to put that information on a legacy Exposure), but it preserves everything else.

TallJimbo force-pushed the tickets/DM-54556 branch 2 times, most recently from d25761e to 46884ab Compare May 21, 2026 19:15

timj approved these changes May 21, 2026

View reviewed changes

TallJimbo force-pushed the tickets/DM-54556 branch from f5ee16e to 9df43a6 Compare May 22, 2026 16:32

TallJimbo commented May 22, 2026

View reviewed changes

timj approved these changes May 22, 2026

View reviewed changes

TallJimbo force-pushed the tickets/DM-54556 branch from 9df43a6 to c0224b8 Compare May 22, 2026 18:07

TallJimbo added 15 commits May 22, 2026 14:39

Move private helpers to the bottom of the file.

7a0e519

Stop using Any for legacy types in image classes.

20fe37b

Since these aren't typed upstream, this does very little for static analysis, but Sphinx uses these types, too, and it's just a better pattern to establish.

Remove unnecessary test import guards.

1ced01e

None of this test would run without afw.

Add tests for existing to_legacy conversions inside Butler.

81834f9

Silence some spurious test warnings.

9bff43d

Add property to identify spatially constant Fields.

667eb78

Add ObservationSummaryStats.to_legacy.

7ba1595

Add PiffWrapper.to_legacy.

47dea14

Add band and ID to VisitImage.

456af68

'band' is a new required attribute. 'ID' is just stuffed into the metadata for now - we'd like to retire it someday, but as long as we need to be able to roundtrip a legacy VisitImage we'll need to carry it around.

Propagate background through VisitImage.convert_unit.

a5ad0c5

Add VisitImage.to_legacy.

19bce8c

Test improvements to obs_base formatter conversions.

2eb7fd7

This provides testing for code in obs_base.

Delete now-unused test helper module.

c1dc532

All code using this module was removed on DM-54976.

Add changelog entry.

0ade457

TallJimbo force-pushed the tickets/DM-54556 branch from c0224b8 to e35eccb Compare May 22, 2026 18:39

TallJimbo force-pushed the tickets/DM-54556 branch from e35eccb to 58cfa82 Compare May 22, 2026 19:07

TallJimbo merged commit 9e1bbef into main May 22, 2026
15 of 17 checks passed

TallJimbo deleted the tickets/DM-54556 branch May 22, 2026 19:12

Conversation

TallJimbo commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

codecov Bot commented May 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

timj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TallJimbo May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TallJimbo May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

TallJimbo commented May 20, 2026 •

edited

Loading

codecov Bot commented May 20, 2026 •

edited

Loading

TallJimbo May 22, 2026 •

edited

Loading

TallJimbo May 22, 2026 •

edited

Loading