DM-54912: add photometric_scaling to VisitImage by TallJimbo · Pull Request #39 · lsst/images

TallJimbo · 2026-05-15T21:17:27Z

Checklist

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

codecov · 2026-05-15T21:18:23Z

Codecov Report

❌ Patch coverage is 17.46725% with 189 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.01%. Comparing base (7121ff8) to head (4a3c4db).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
tests/test_visit_image.py	19.04%	68 Missing ⚠️
python/lsst/images/_visit_image.py	19.75%	65 Missing ⚠️
python/lsst/images/tests/_checks.py	11.76%	15 Missing ⚠️
python/lsst/images/fields/_chebyshev.py	0.00%	11 Missing ⚠️
python/lsst/images/fields/_concrete.py	25.00%	9 Missing ⚠️
python/lsst/images/_image.py	16.66%	5 Missing ⚠️
python/lsst/images/fits/_common.py	20.00%	4 Missing ⚠️
...thon/lsst/images/tests/extract_legacy_test_data.py	0.00%	4 Missing ⚠️
python/lsst/images/_masked_image.py	0.00%	3 Missing ⚠️
python/lsst/images/fields/_product.py	0.00%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #39      +/-   ##
==========================================
- Coverage   76.11%   75.01%   -1.10%     
==========================================
  Files          91       91              
  Lines       10607    10805     +198     
==========================================
+ Hits         8073     8105      +32     
- Misses       2534     2700     +166

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

TallJimbo · 2026-05-15T21:24:18Z

Note to self: if this lands before #37, update the docs there. If it lands after, update the docs here.

natelust

There are lots of things to consider as quick design thoughts. I am pretty ok with whatever you come up with for an answer, but I wanted to bring things up before it gets baked in forever. I do think some of the nests of branches are a bit of a rats nest, but I am not sure if there is a create alternative.

natelust · 2026-05-19T14:46:52Z

+        unit: astropy.units.UnitBase | None = None,
+        bounds: Bounds | None = None,
    ) -> ChebyshevField:
        """Convert from a legacy `lsst.afw.math.ChebyshevBoundedField`."""


I think these need proper docstrings, someone running this would have no idea what the other things are without reading the code.

natelust · 2026-05-19T14:47:52Z

+    bounds: Bounds | None = None,
 ) -> Field:
    """Convert a legacy `lsst.afw.math.BoundedField` subclass to a `BaseField`
    object.


same complaint here

natelust · 2026-05-19T14:49:59Z

@@ -70,9 +77,9 @@ def field_from_legacy(

    match legacy_bounded_field:
        case ChebyshevBoundedField():


I'm 99% sure if you wanted to remove the (), you could just do afwMath.ChebyshevBoundedField , etc. Might make it clearer too, but the import is right above so not needed, just wanted to bring up the stylistic choice.

That would actually turn this into an issubclass check instead of an isinstance check.

natelust · 2026-05-19T15:00:44Z

+def field_from_legacy_photo_calib(
+    legacy_photo_calib: LegacyPhotoCalib,
+    bounds: Bounds,
+    post_isr_unit: astropy.units.UnitBase = astropy.units.electron,


It seems a little weird to bake this directly into the parameter name

I've been wondering about calling it instrumental_unit instead, and it sounds like I should do that (note that I can't just a call it "unit" because the full unit is something like nJy/electron; it's just the denominator I'm trying to name).

natelust · 2026-05-19T15:02:07Z

+    bounds: Bounds,
+    post_isr_unit: astropy.units.UnitBase = astropy.units.electron,
+) -> Field | None:
+    """Convert a legacy `lsst.afw.image.PhotoCalib` into a `BaseField` object.


I strongly dislike the name BaseField to represent some kind of unit conversion. It makes it seem like it is the base class instance for all Fields. Or is it that, and you didn't specialize anything?

This is indeed the base class for all fields. It's confusing because BaseField is the ABC, while Field is a type alias to a union of all the concrete field types; we need to use the former in docs (because Sphinx can't handle type aliases) but we want to use the latter in annotations to get better static analysis.

In afw, PhotoCalib is a nonpolymorphic class that has a BoundedField, but it doesn't really add anything on top of it except an expectation of the units. And here all the field types have explicit units, so I decided to just not have that extra level of indirection.

but later if you need that concept in some other sense it gets ambiguous, and I can't really ask the interpreter (type) to understand what the thing is if I have one

I'm just pretty confident we're not going to need that concept in the future. I don't think it's fundamentally different from using an Image to represent both [the image plane of] science data and a flat field.

And you can't think of anytime there would be Field types that would not be appropriate for being a photoclaib?

There might be some that wouldn't be very useful - e.g. since it's multiplicative, I don't really see a sum of fields ever being used. But I don't think there could ever be one that actually couldn't be used because it would be problematic. The whole point of the BaseField hierarchy is that you don't care what subclass you get, and we only have an enumerated closed hierarchy for serialization (i.e. because otherwise you need a plugin system to deserialize and that gets really messy).

natelust · 2026-05-19T15:14:56Z

            compare_aperture_corrections_to_legacy(
                tc, aperture_corrections, legacy_exposure.info.getApCorrMap(), tiny_bbox
            )
+        if (photometric_scaling := alternates.get("photometic_scaling", ...)) is not ...:


Is this because None is a valid return type?

natelust · 2026-05-19T15:22:31Z

+    legacy_exposure: Any
+    plane_map: dict[str, MaskPlane]
+    visit_image: VisitImage
+    unit: u.UnitBase


Is this actually a mixing for final data classes, so you specify these as class attribute here, or are they really class attributes?

Or are you just setting these for typing reasons?

Mostly just typing reasons (even though this file isn't formally type-checked, I like reducing editor squiggles where I can).

natelust · 2026-05-19T15:28:34Z

+    Requires legacy code.
+    """
+
+    @classmethod


This seems like an extremely strange pattern of class creation. Why does this need to be class level, is it to stop multiple reads or something? Why does this not become standard class instantiation or something?

This is how you get unittest to do test-setup only once (or at least only once per process). It is indeed to reduce I/O in this case.

I don't really know why unittest doesn't use constructors for initialization; presumably they wanted to use them for something else. But that's the pattern.

natelust · 2026-05-19T15:31:09Z

+        return self._photometric_scaling
+
+    @photometric_scaling.setter
+    def photometric_scaling(self, value: Field | None):


Is it really true all Field s are valid?

Yes. This is a deliberate change from afw (all fields were valid there, too, but PhotoCalib added another level of indirection that I'm deliberately dropping).

natelust · 2026-05-19T15:33:31Z

+            An image with the given units.  Will always share most attributes
+            with self, with the `image` and `variance` copied only if they
+            need to be modified (i.e. they do not already have the right
+            units).


A sometimes copied method is worse than a never copied or always copied method in my opinion, but I get it might be an efficiency thing, but I can see this becoming no end a source of bugs for us in the future.

I'd usually very much agree. I've done it this way here because I think it's going to be used almost all of the time in a

visit_image = visit_image.convert_unit(...)

sense where you blow away what you had before and are really wanting to simulate an in-place operation as much as possible for efficiency. But we can't do that directly because in-place changing the units of an array that might be shared is an even worse problem (IMO).

I'm really not sure I agree on this one. I agree with the reasoning of what you said, but this is such a giant gaping hole in an otherwise really great api I think that something somewhere has been overlooked or there has to be a better pattern. I see what you said in your example, but I can also see someone doing:

converted_image = visit_image.conver_unit(...) function_on_new(converted_image) # mutating use_old_image(visit_image)

I think what I'm missing is a better alternative. The only one I can think of is to always copy everything by default, with a kwarg to switch back to the current behavior (which I want to preserve as an option because I know it's the right one for the use case I know about). That would make the behavior a lot more explicit, but more verbose in the usual case.

The only other thing I can think of is an abstraction with COW

I tried a few things before things got to this stage, and I think I've convinced myself that COW is just impossible with Numpy.

Anyhow, I've switched to the kwarg control proposed above. I worry a bit that it's extra logic to maintain that won't get used (i.e. we'll always pass the options to make it behave like it was before), but even in that case it is easier to read the code and see what's going on.

MyPy is having a hard time working out that if x in (a, b): if x == a: return implies x != a

TallJimbo mentioned this pull request May 15, 2026

DM-54935: Add draft of doc section on transitioning from afw. #37

Open

2 tasks

natelust approved these changes May 19, 2026

View reviewed changes

TallJimbo force-pushed the tickets/DM-54912 branch from 0beed28 to 5a5aa4f Compare May 19, 2026 17:35

TallJimbo added 12 commits May 19, 2026 13:56

Add field_from_legacy_photo_calib converter function.

9ccd070

Add photometric scaling component to VisitImage.

89890e8

Get post-ISR units from LSST-specific headers and fix bad BUNIT.

5af2408

Run visit image legacy tests on a DR preliminary_visit_image, too.

d3847ab

Drop some unnecessary whitespace.

2b5e30a

Add a visit_summary catalog to test data extraction.

1f9b18d

Stop unnecessarily copying Image and Mask arrays at construction.

88e671c

Add method to convert VisitImage units.

2ced257

Add changelog entry.

3a6f596

Appease MyPy's lack of cleverness with harmless de-indent.

bac0e7c

MyPy is having a hard time working out that if x in (a, b): if x == a: return implies x != a

Make field from_legacy conversions more consistent.

27ce0ea

Rename post_isr_unit args/vars to instrumental_unit.

ff38799

TallJimbo force-pushed the tickets/DM-54912 branch from 9949daa to ff38799 Compare May 19, 2026 17:56

Make VisitImage.convert_unit copy by default.

4a3c4db

TallJimbo force-pushed the tickets/DM-54912 branch from 7e5d2d2 to 4a3c4db Compare May 19, 2026 19:17

TallJimbo merged commit ef08fe4 into main May 19, 2026
17 of 19 checks passed

TallJimbo deleted the tickets/DM-54912 branch May 19, 2026 21:45

		@@ -70,9 +77,9 @@ def field_from_legacy(

		match legacy_bounded_field:
		case ChebyshevBoundedField():

Conversation

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

Uh oh!

Checklist

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TallJimbo commented May 15, 2026

Uh oh!

natelust 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!

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 19, 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!

TallJimbo May 19, 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!

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!

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!

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!

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

TallJimbo commented May 15, 2026 •

edited

Loading

codecov Bot commented May 15, 2026 •

edited

Loading

TallJimbo May 19, 2026 •

edited

Loading

TallJimbo May 19, 2026 •

edited

Loading