Initial implementation of Roman PSF by rmjarvis · Pull Request #195 · rmjarvis/Piff

rmjarvis · 2026-03-19T20:51:02Z

I think this set of commits is pretty much the minimum set of changes to implement the Roman PSF type. It's not quite everything that I have in the roman branch, but the main swaths of the code are here.

Things still to come in later PRs (to help ease the code review burden):

aberration_interp="linear" option
Running the SCAs in multiple processes
chromatic=True (This technically has it, but it is too inefficient to be useful.)
Misc changes I made to diagnostics and other parts of the code along the way.

…ed correctly.

…ighter prior

arunkannawadi

I've taken only a quick pass for now, but wanted to leave the comments I have so far.

arunkannawadi · 2026-03-20T17:05:35Z


 # Optics
 from .optical_model import Optical, optical_templates
+from .roman_psf import Roman, RomanOptics


Unlike other items, this seems very Roman-specific. Do we want this in our namespace by default? Could we do something like you have for des below?

Sure. That's a good idea. We can have a roman directory with this file and any others we decide to add relevant to the Roman PSF.

arunkannawadi · 2026-03-20T17:15:59Z

+        )
+
+
+class Roman(Model):


I find the naming unintuitive. If I saw a bare Roman class in code, I wouldn't know what that's actually referring to. RomanPSF would be better given the docstring.

Agreed. These aren't great names. How about RomanOpticalModel and RomanOpticalPSF.

I think I prefer RomanOptics over RomanOptical for the written type in the config file, so I think RomanOpticsPSF for the class name works better. But RomanOpticalModel is still good for the model class name.

arunkannawadi · 2026-03-20T17:17:24Z

+        )
+
+
+class RomanOptics(PSF):


Given that this is subclassing PSF, I wonder if this should be RomanOpticalPSF or something like that. I see other examples of the parent class' name appearing, like RomanSCAInterp when it inherits from Interp.

nihardalal

A few small changes here and there, feel free to resolve as necessary. ~~Also, in running the pytest for this branch, I also got an error in test_input.py in test_stars() where assert len(stars) == 90 fails on line 1683 under the "gratuitous coverage test"~~

I was running the tests with the wrong environment - all tests passed on my end with the correct environment, sorry for the confusion

nihardalal · 2026-03-20T00:34:20Z

+select:
+
+    min_snr: 50           # reject very faint stars
+    #max_snr: 500          # don't over-weight very bright stars


Could change max_snr to max_snr_weight - I think this gets the main point of the parameter across better

Suggested change

#max_snr: 500 # don't over-weight very bright stars

#max_snr_weight: 500 # don't over-weight very bright stars

Good idea. I like this name. I'll keep the other one as deprecated, but I agree this is a better name.

nihardalal · 2026-03-20T14:00:40Z

+        mean = np.array([self.sca_mean[s] for s in sca])
+        data = np.array(
+            list(zip(sca, mean)),
+            dtype=[('sca', int), ('mean', float, (len(self.global_mean),))],


This line confused me at first - might be good to document to say "writes an n_param length array for each sca"

nihardalal · 2026-03-20T14:05:59Z

+    each SCA. For each ``(sca, params)`` state we build corner PSFs at the four SCA corners and
+    interpolate between them at each star position. This keeps the optical model much faster when
+    many stars on a chip share similar parameter vectors.
+    """


Seems like a reasonable strategy. I do know that Cycle9 Zernikes are actually measured at 5 points across the chip - 4 corners and the center. Does it make sense to use all 5 points and do some kind of grid interpolation? (I guess the improvement is marginal, but its nice to have a one to one between the cycle9 data and the drawing in this case)

OK. I could think about how to interpolate that. 5 is a bit awkward for interpolation. I guess you get a single quadratic term? 4 gives you (1,x,y,xy). 5 gives either (1,x,y,x^2,y^2) or (1,x,y,xy,x^2+y^2) I guess. Or we could do linear in each triangle?

Here's a function that we use in psfsim to interpolate the Zernikes. I think scipy.interpolate.griddata can also work

def altgriddata(points, values, xi): """ Quadratic fitting function. This is a substitute for ``scipy.interpolate.griddata`` when we have 5 points and want to fit a quadratic, minus the x^2-y^2 term. Paramters --------- points : np.ndarray of float The coordinates of the 5 points where we have values; shape (5, 2). values : np.ndarray of flat The values to interpolate; shape (5,). xi : (float, float) The location to interpolate to. Returns ------- float The interpolated value. """ # Set up the linear system. # Order of coefficients is 1, x, y, x*y, x^2+y^2. bval = np.zeros((5, 5)) # bval[i,j] is the ith basis function at jth point bval[0, :] = 1.0 bval[1, :] = points[:, 0] bval[2, :] = points[:, 1] bval[3, :] = points[:, 0] * points[:, 1] bval[4, :] = points[:, 0] ** 2 + points[:, 1] ** 2 mat = bval @ bval.T vec = bval @ values coefs = np.linalg.solve(mat, vec) return ( coefs[0] + coefs[1] * xi[0] + coefs[2] * xi[1] + coefs[3] * xi[0] * xi[1] + coefs[4] * (xi[0] ** 2 + xi[1] ** 2) )

Right. Thanks. So (1,x,y,xy,x^2+y^2) then. I can implement that. I'll do it in a different PR though.

nihardalal · 2026-03-20T18:33:06Z

+        assert isinstance(psf2.interp, RomanSCAInterp)
+        assert psf2.interp.global_mean is None
+        assert psf2.interp.sca_mean == {}
+


I think these tests look okay. I haven't been able to check for comprehensive coverage given the size of the file, but I think something like codecov would do a better job than me.

I do use codecov. Looks like I missed one line of coverage when I pulled out this subset. I think the roman branch has 100% coverage. But I guess I missed one test line when I was assembling this subset.

cf. https://github.com/rmjarvis/Piff/pull/195/checks?check_run_id=67957433502

Co-authored-by: Nihar Dalal <140463192+nihardalal@users.noreply.github.com>

nihardalal

I think everything looks okay now, so good to merge whenever

rmjarvis · 2026-03-23T14:39:59Z

I think I'm up to date with responding to suggestions.
@arunkannawadi Did you want to look at this some more before I merge?

arunkannawadi · 2026-03-23T18:18:53Z

I can take another look, but if I don't get back to you by EOD tomorrow, don't wait up for me.

rmjarvis added 9 commits March 19, 2026 16:19

Make sure heterogeneous params_var items in composite PSFs get record…

59ffc93

…ed correctly.

First pass at RomanOptics PSF type

0edf5ed

Update to use bilinear interpolation of PSF within each SCA

c9c9569

Fix per_sca=False option

e5c76e8

Clean up the test suite some

d276a73

Make prior=0.05 the default. 1.0 was way too big.

86d1ca6

Switch to aberration_interp API

91f2ca2

Use multiple outliers in Roman test

81da0c2

Update roman piff.yaml to use Centroid outlier, max_sep, nproc, and t…

8db46a5

…ighter prior

arunkannawadi self-requested a review March 19, 2026 23:04

nihardalal self-assigned this Mar 20, 2026

nihardalal self-requested a review March 20, 2026 00:31

arunkannawadi reviewed Mar 20, 2026

View reviewed changes

nihardalal requested changes Mar 20, 2026

View reviewed changes

rmjarvis and others added 7 commits March 20, 2026 15:11

Update devel/roman/piff.yaml

ed0880f

Co-authored-by: Nihar Dalal <140463192+nihardalal@users.noreply.github.com>

Update piff/roman_psf.py

75d7de0

Co-authored-by: Nihar Dalal <140463192+nihardalal@users.noreply.github.com>

Update piff/roman_psf.py

bf32c78

Co-authored-by: Nihar Dalal <140463192+nihardalal@users.noreply.github.com>

Move roman_psf.py to roman directory and require explicit piff.roman.*

4b4d31d

Update class names to RomanOpticsPSF and RomanOpticalModel

e3dac91

Rename max_snr -> max_snr_weight

4584e11

Add explanatory comment

6d66b0d

nihardalal approved these changes Mar 20, 2026

View reviewed changes

rmjarvis added 4 commits March 20, 2026 16:15

coverage

6e4bd42

Forgot to commit __init__.py

54a9555

coverage

63fdc33

coverage

d48edc3

Use max_snr_weight in devel/roman piff.yaml

dca45da

rmjarvis merged commit 6190aa3 into main Mar 25, 2026
9 checks passed

rmjarvis deleted the roman1 branch March 25, 2026 21:03

	#max_snr: 500 # don't over-weight very bright stars
	#max_snr_weight: 500 # don't over-weight very bright stars

Conversation

rmjarvis commented Mar 19, 2026

Uh oh!

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

nihardalal left a comment • 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!

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!

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!

nihardalal left a comment

Choose a reason for hiding this comment

Uh oh!

rmjarvis commented Mar 23, 2026

Uh oh!

arunkannawadi commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nihardalal left a comment •

edited

Loading