Add two options for how aberrations vary over SCAs by rmjarvis · Pull Request #197 · rmjarvis/Piff

rmjarvis · 2026-04-01T22:47:44Z

The PR adds two features to the RomanOptics PSF class.

nominal_interp = "five_points". The project supplied nominal aberrations on each SCA are supplied at 5 points for each SCA: the four corners plus the center of the SCA. The default pattern for approximating the PSF in Piff is to just use the 4 corner values and take the PSF at any other position to be a weighted sum of these. @nihardalal pointed out that we could use all 5 locations where the project gives us information, including the central point instead. So setting this parameter to five_points rather than bilinear (still the default) does that.

Piff does this a little bit different from what GalSim does when calculating the PSF at an arbitrary location. GalSim does a bilinear interpolation of the aberrations themselves, and then computes the PSF from that. Piff computes the 4 or 5 PSFs with the project aberration values and then interpolates the whole PSF. I don't really have a good feel for which is more accurate. But the way Piff does it is computationally much more tractable, since constructing the PSF is relatively expensive, so using the interpolated (either bilinear or five_points) PSF is much faster.

In my test exposure, bilinear performed slightly better than five_points, but there wasn't much difference. We'll see how this goes on the real data eventually. I think this will at least be something we'll want to be able to test as to which one gives better results.

aberration_interp = linear. In addition to the nominal aberrations, Piff fits for an offset to these. An "extra_aberrations" array. The default is to have a single extra_aberrations array for each SCA. This is aberration_interp="constant", and that is the default. This PR adds the option to have them be a linear function of x,y across the SCA. This option gave a worse fit on the test exposure I've been using, so constant is still the default, but we will definitely want to have this option available to us when testing the models on real data. The issue is that there are 3x as many degrees of freedom with this fit, and the data aren't strongly constraining. (The prior needs to be quite tight to avoid it going off the rails.) But with real data, we may be able to have very tight priors on linear patterns based on an ensemble of exposures, in which case this option may become more viable.

… shows reasonable profiles and residuals.

…put file

…t the center from the project aberration file.

… the dev sim.

rmjarvis · 2026-04-02T04:06:46Z

By the way, no need to do a careful code review of the two diagnostic scripts in the devel/roman directory. These are scripts that Codex wrote for evaluating the quality of the model solutions for the piff.yaml runs there.

nihardalal

Looks good to go. I think you caught all the variable renames, and the unit tests seem pretty good to me as well. As I mentioned in the in-line reviews, we should definitely do more testing of five-points vs bilinear (either on other exposures or real data)

nihardalal · 2026-04-02T15:15:09Z

Did a brief scan, looks fine!

nihardalal · 2026-04-02T15:15:56Z


    min_snr: 50           # reject very faint stars
    max_snr_weight: 500   # don't over-weight very bright stars
-    #max_mask_pixels: 10   # how many pixels in stamp are allowed to be masked.


Did you mean for this comment to be removed?

Yeah, I had tried it a long time ago, and it wasn't helpful. So I commented it out.
I finally decided it wasn't worth keeping here as an advertisement that it was possible. My guess is we won't want it for Roman. I haven't been using it for this test run.

nihardalal · 2026-04-02T15:22:48Z

    def clear_cache(self):
        self._corner_cache = {}

+    def _get_roman_five_point_data(self, sca):


Makes sense to me!

nihardalal · 2026-04-02T15:30:29Z

+        for i, (pt, p) in enumerate(zip(points, sample_params)):
+            extra = self._make_extra_aberrations(p)
+            if self.nominal_interp == 'five_point' and i == 4:
+                extra = extra + self._get_roman_five_point_data(sca)['center_delta']


Looks correct to me - indexing seems fine/taken proper care of

nihardalal · 2026-04-02T15:31:49Z

            fx * fy,
        )

+    def _five_point_weights(self, star):


Looks correct!

nihardalal · 2026-04-02T15:38:33Z

+
+
+@timer
+def test_five_point_weights():


This test looks fine to me!

nihardalal · 2026-04-02T15:41:02Z

-            ),
-        )
-        fitted = model.fit(fit_star)
+        prof = model.getProfile(truth_params, star=star)


This is a lot more compact and readable, good change

nihardalal · 2026-04-02T15:47:46Z

+        aberration_prior_sigma=[0.1, 0.2, 0.3],
+    )
+    np.testing.assert_allclose(linear_vec.prior_sigma, np.tile([0.1, 0.2, 0.3], 3))
+


Looks like a comprehensive test set

nihardalal · 2026-04-02T17:43:31Z


 @timer
-def test_roman_fit_many():
+def test_linear_prior_io():


Looks good to me!

nihardalal · 2026-04-02T17:50:08Z

+
+        # With direct GalSim truth generation, both nominal interpolation options should converge
+        # to reasonable fits, but neither is expected to recover truth_params exactly
+        # for the reason mentioned above.  (Profile interpolation != aberration interpolation.)


We should definitely do some more investigation into this (worse fit but better chi^2) in the near future

This wasn't the case in the big devel/roman test. There the overall chisq was worse with five_point. Not outrageously so, but enough that I left the canonical value there as bilinear.

rmjarvis · 2026-04-03T16:57:57Z

Thanks Nihar! Merging.

rmjarvis added 16 commits April 1, 2026 17:31

Add aberration_interp=linear option

7c503b1

This version of the code and config gives pretty good results. Run 11…

23b7f37

… shows reasonable profiles and residuals.

Fix serialization bug when using linear

5bd05a5

Add a diagnostic script for analyzing the results of a Roman piff out…

b5a6575

…put file

More diagnostics in diagnose_piff.py

d615332

add script to compare bilinear approx vs direct getPSF usage

30742ea

gitignore AGENTS.md

c2182f6

Add nominal_inter='five_points' option

d41e33b

Fix five-point implementation to correctly get the right aberration a…

7dc89ac

…t the center from the project aberration file.

Add annotation that bilinear seems to work better than five_points on…

f04662b

… the dev sim.

Remove redundant roman from many roman test names

30feaae

Simplify structure of many tests in test_roman.py

c7dde78

Incorporate five_points option in test_fit_linear

23994d6

Switch aberration_interp=linear to mean actual linear, not bilinear

c5751fc

Go back to full version of piff.yaml run in devel/roman

1266575

simplify import line

ed6a10f

nihardalal self-requested a review April 2, 2026 01:02

restore comment that got removed by accident

fc99767

nihardalal approved these changes Apr 2, 2026

View reviewed changes

rmjarvis merged commit 2ae505e into main Apr 3, 2026
9 checks passed

rmjarvis deleted the roman3 branch April 3, 2026 16:58

Conversation

rmjarvis commented Apr 1, 2026

Uh oh!

rmjarvis commented Apr 2, 2026

Uh oh!

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

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!

rmjarvis commented Apr 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants