DM-54555: Switch brightStarCutout to lsst.images output by leeskelvin · Pull Request #1293 · lsst/pipe_tasks

leeskelvin · 2026-04-09T16:39:47Z

No description provided.

TallJimbo · 2026-04-15T13:56:26Z

+                    detector=input_exposure.detector.getId(),
+                    bbox=Box.from_legacy(stamp_bbox),
+                ),
+            )


Because this Projection is for the pixels in your special star-centered, focal-plane-radial frame, I don't think DetectorFrame is appropriate here.

Unfortunately, I don't think we have a Frame type in lsst.images that would be appropriate, and that's not something the library is designed to let you define outside of it.

I've been sort of anticipating that we'd hit this limitation sooner or later (I know the current set of Frame types isn't complete), but I wanted to have at least one concrete use case before trying to solve the problem. Since this is such a niche use case, I don't think we want to add a Frame type that exactly describes it. A Frame type for coordinate systems centered on objects or sources might be appropriate - but I'm not sure what utility it would have, or what it might actually have in common with other coordinate systems that could be described that way. So I think it's probably just time to add a vague GenericFrame (naming ideas welcome) to lsst/images/transforms/_frames.py (sorry; we'll have to revive that PR) to use here.

Switched to:

projection = Projection.from_legacy(stamp_wcs, GeneralFrame(unit=u.pixel))

TallJimbo · 2026-04-15T14:42:50Z

+    def __getitem__(self, bbox: Box | EllipsisType) -> BrightStarStamp:
+        super().__getitem__(bbox)
+        if bbox is ...:
+            return self


This if block needs to move before the super() call, I think. This was a common problem in the lsst.images types that we recently fixed.

TallJimbo · 2026-04-15T14:43:48Z

+        )
+
+    @property
+    def psf(self) -> Image:


Might be best to rename this to psf_kernel_image so it's not as easily confused with attributes of other types that return an actual PointSpreadFunction instance.

TallJimbo · 2026-04-15T14:51:51Z

+            projection=projection,
+            obs_info=obs_info,
+            metadata=metadata,
+        )


It turns out Image and Mask can hold an ObservationInfo directly, too, so it's possible that this model is going to end up duplicating that information on serialization.

But looking deeper, this is mostly an upstream problem; we should be treating the ObservationInfo in MaskedImage more like we treat Projection - making sure we set it on all component images at construction, and then telling the components not to save it in serialize. And we're very inconsistent about that in both MaskedImage and VisitImage. I think if that got fixed, you could remove obs_info from your serialization model here, because you'd get one on the MaskedImage serialization model.

I'd like to get this all worked out before you start saving things in the new format, but I don't want to keep blocking you. Since I think you'll need an lsst.images branch for this ticket after all - for the frame issue - how about I add a commit fixing all of these upstream issues to that branch, and then adjusting here downstream?

That sounds good, please do make use of the prior lsst.images branch if helpful. More than happy to wait a little longer to get this right from the outset.

This led to a fair amount of simplification inside brightStarStamps.py beyond just obs_info, thank you.

TallJimbo · 2026-04-15T14:58:31Z

+        filename : `str`
+            Name of the FITS file to write.
+        """
+        fits.write(self, filename)


Per comment on daf_butler, I'm hoping you can switch the formatter to lsst.images.fits.GenericFormatter and then drop readFits[WithOptions] and writeFits.

Switched to lsst.images.fits.formatters.GenericFormatter, which I think was the intent? (Rather than modifying the __init__ to raise GenericFormatter into fits).

TallJimbo · 2026-04-15T14:59:57Z

+
+        with NamedTemporaryFile() as file:
+            self.bright_star_stamps.writeFits(file.name)
+            bright_star_stamps = BrightStarStamps.readFits(file.name)


If you want to test the butler support here as well, rewrite this as:

with lsst.images.tests.RoundtripFits(self.bright_star_stamps, storageClass="BrightStarStamps") as roundtrip: pass bright_star_stamps = roundtrip.result

You can also use roundtrip.inspect() inside the context manager to poke at what the FITS file actually looks like with astropy.io.fits if you want to test that as well.

This script was previously located in meas_algorithms. The decision to move it into pipe_tasks was two-fold: 1. Use of lsst.images format inside bright star stamps introduced a cyclical dependency issue. lsst.images depends on meas_extensions_psfex and meas_extensions_piff (optionally) because it has wrappers for those and needs to be able to import the legacy modules to test them. Moving the bright star stamp storage classes into pipe_tasks resolves this issue in the short term. 2. Having the storage classes located alongside the consuming pipeline tasks helps to streamline development, keeping edits contained within a single package.

With the prior commit, the remit for this namespace has changed from a space to hold the bright star subtraction pipeline tasks to a location for all tasks related to bright stars. This namespace now contains the bright star stamp and stamps storage classes, as well as tools to construct extended PSF models.

To avoid potential confusion with the existing stamps storage classes, we rename all instances of "BrightStarStamp" and similar on this commit. Changes include: - `BrightStarCutoutTask` -> `ExtendedPsfCutoutTask` - `brightStars` -> `extendedPsf` - `brightStarStamps` -> `extendedPsfCandidates` - etc.

TallJimbo · 2026-04-17T16:11:05Z

+        attrs = ", ".join(f"{k}={v!r}" for k, v in self.__dict__.items())
+        return f"BrightStarStampInfo({attrs})"
+
+    __repr__ = __str__


If you define __repr__ instead, str will automatically delegate to it.

In this case, it seems not though. For my testing on an epc.star_info object, I have:

epc = epcs[0] print(epc.star_info) epc.star_info

returning:

visit=2025102600226 detector=178 ref_id=4980307157180392448 ref_mag=16.378769863064512 position_x=3485.5599400703168 position_y=2400.2426411600445 focal_plane_radius=<Quantity 300.59689709 mm> focal_plane_angle=<Quantity 1.52257155 rad> ExtendedPsfCandidateInfo(visit=2025102600226, detector=178, ref_id=4980307157180392448, ref_mag=16.378769863064512, position_x=3485.5599400703168, position_y=2400.2426411600445, focal_plane_radius=<Quantity 300.59689709 mm>, focal_plane_angle=<Quantity 1.52257155 rad>)

It looks like the delegation rule is only true when the inherited implementation is object.str. I forgot why I did it this way originally, but revisiting it now, I realized that I don't like the default pydantic __str__, without any commas between the items, nor identifying the storage class name at the beginning.

Ah, that makes sense; I wasn't think about whether the __str__ Pydantic provided.

leeskelvin · 2026-04-17T16:40:45Z

Thanks for the review comments above @TallJimbo. A fair amount of reshuffling has occurred at the end of this PR. I initially switched from brightStar* to extendedPsf* everywhere, but then it occurred to me that perhaps namespaces and file names should also be in snake_case instead of camelCase (i.e., extended_psf). I changed the namespaces and filenames to snake_case on a subsequent commit, making it easy to revert if you prefer the original layout.

Other than that, as discussed, I've removed the legacy extended PSF code here as well. And, as a last-minute addition, I added read_/write_fits methods for the ExtendedPsfCandidates class as well. I've tested that these outputs can be saved and read back in successfully in a local notebook.

TallJimbo · 2026-04-17T16:47:37Z

👍, all sounds good. Subpackage boundaries seem like a perfectly good place to switch to snake_case, and I think matching the style of the code within the subpackage is a good idea.

leeskelvin force-pushed the tickets/DM-54555 branch 2 times, most recently from 0a502ad to 958e769 Compare April 9, 2026 19:25

leeskelvin changed the title ~~DM-54555: Switch to lsst.images output~~ DM-54555: Switch brightStarCutout to lsst.images output Apr 9, 2026

leeskelvin force-pushed the tickets/DM-54555 branch 4 times, most recently from 6843815 to 3669f29 Compare April 14, 2026 19:25

TallJimbo reviewed Apr 15, 2026

View reviewed changes

leeskelvin added 5 commits April 16, 2026 07:01

Switch brightStarCutout to lsst.images output

5004187

Apply cosmetic edits and typo fixes

3107c44

Update bright star subtraction unit tests

41473ef

leeskelvin force-pushed the tickets/DM-54555 branch from 3669f29 to 5498f37 Compare April 16, 2026 18:36

leeskelvin added 4 commits April 17, 2026 08:05

Rename from camelCase to snake_case

70e25ee

Remove legacy bright star subtraction code

d58b3fe

Add read/write FITS to ExtendedPsfCandidates

24695c6

leeskelvin force-pushed the tickets/DM-54555 branch from 5498f37 to 24695c6 Compare April 17, 2026 16:11

TallJimbo approved these changes Apr 17, 2026

View reviewed changes

Conversation

leeskelvin commented Apr 9, 2026

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!

leeskelvin commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TallJimbo commented Apr 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leeskelvin commented Apr 17, 2026 •

edited

Loading