Add modelSize to piff config #23

esheldon · 2024-04-04T18:01:26Z

add modelSize to piff config

The modelSize is the internal PIFF model grid size.  This gets
sent to the piff config rather than stampSize for the "size" entry.

modelSize defaults to 25. Update the stampSize to default to sqrt(2) * 25 ~ 35
to accomodate rotations

Add a check that stamp size is big enough

update tests to include rotation

This is an important check when working in sky coords

Oddly at 45 degrees piff is giving a singlular matrix error
and fails to fit.  We should track this down before accepting
this PR.  135 works fine so I wonder if there is something
sepcial about 45 for this test

The modelSize is the internal PIFF model grid size. This gets sent to the piff config rather than stampSize for the "size" entry. modelSize defaults to 25. Update the stampSize to default to sqrt(2) * 25 ~ 35 to accomodate rotations Add a check that stamp size is big enough

This is an important check when working in sky coords Oddly at 45 degrees piff is giving a singlular matrix error and fails to fit. We should track this down before accepting this PR. 135 works fine so I wonder if there is something sepcial about 45 for this test

Will work for FITS wcs

esheldon · 2024-04-05T18:28:15Z

The unit tests pass for me. Do we turn on the CI here or is that to be done later?

We think its an issue with the underlying galsim code rather than the wrapper

esheldon · 2024-04-05T19:28:39Z

There is a fix in galsim that allows the unit tests to pass.

Should we add more test cases?

arunkannawadi · 2024-04-05T19:58:32Z

If by more test cases you mean more angles, I'm happy to do that after merging this PR. But once you're happy with this PR as is, I'll merge it to the ticket branch and run CI tests, but only after the new GalSim release makes it to rubin-env. You'll have an opportunity to review before merging to main.

tests/test_psf.py

arunkannawadi · 2024-04-04T18:21:56Z

python/lsst/meas/extensions/piff/piffPsfDeterminer.py

+            f'we require stampSize >= '
+            f'{min_stamp_size}, got {stamp_size}'
+        )
+


Note to self: this is a check only on the config, so should be implemented in a validate method.

python/lsst/meas/extensions/piff/piffPsfDeterminer.py

arunkannawadi · 2024-04-12T13:06:33Z

Thanks again for this PR, Erin. If there are no more changes you're planning, I'll go ahead and merge it.

esheldon · 2024-04-12T13:11:41Z

No more changes

arunkannawadi · 2024-04-12T13:14:58Z

python/lsst/meas/extensions/piff/piffPsfDeterminer.py

        # i) aperture flux with 12 pixel radius can be compared to PSF flux.
        # ii) fake sources injected to match the 12 pixel aperture flux get
        #     measured correctly
-        self.stampSize = 25
+        # Default modelSize is 25, and 35 is about sqrt(2) * 25
+        self.stampSize = 35


Given that in the default processing we don't use sky coordinates, I'm inclined to keep this as 25 in the default here. For your testing purposes, you should be able to override this value.

@rmjarvis Is there any disadvantage to having modelSize == stampSize?

Yes. You definitely don't want stampSize == modelSize. It should be larger for 3 reasons.

The Lanczos interpolation extends the active modeling space out from the modelSize by interpolant.xrange(). I think your default is Lanczos(11), so it will reach out by 11 pixels on all sides. That alone suggests you want stampSize = modelSize + 22.

Even if your centroids are absolutely perfect, there is a sub-pixel variation of half a pixel in some direction. So you would want to add one more pixel of data around the edges to make sure you are properly constraining all the pixels around the edge. That adds another +2 to the stamp size. Maybe a little more if you don't trust your centroids to be very sub-pixel accurate.

Finally, the rotation. This multiplies that calculation by up to sqrt(2). (1/max(|sin(theta)|,|cos(theta)|) if you want to calculate it explicitly for a given WCS.)

And honestly, there is barely any down side to using a larger than needed stamp size. Piff will just ignore any pixels that are not being used to constrain the model. So I'd recommend the default being much larger in fact. Use 80x80 or something.

The default I put in was 35 which was designed to account for your part 3. in the list, since the default modelSize was set to 25. It seems like 1. and 2. imply we should go even larger

The Lanczos interpolation extends the active modeling space out from the modelSize by interpolant.xrange()

Mike, could you confirm that this is needed even if the modeling is happening in pixel coordinates as opposed to sky coordinates?

"Needed"? No. Not technically. But it will produce inaccurate PSF models near the edges if you don't.

Also, I remember the compute scales as cubic power of the size, but does it as modelSize**3 or stampSize**3? I hope the former.

modelSize**6 actually.

rmjarvis · 2024-04-12T14:43:19Z

python/lsst/meas/extensions/piff/piffPsfDeterminer.py

+    if stamp_size % 2 == 0:
+        raise ValueError(f'stampSize should be odd, got {stamp_size}')
+    if model_size % 2 == 0:
+        raise ValueError(f'modelSize should be odd, got {model_size}')


Why this? I don't think there is any restriction that either of these must be odd. Odd model size is common, but not really necessary. Most PSF profiles have a lot of flux in the central 1 pixel, so odd helps model that better. But there could certainly be cases where even works better. (I did test both odd and even for DES, and the odd sizes in fact worked better in those tests -- I haven't switched to even sizes since then.)
And I usually use even stampSize actually. The default in Piff is 32, which is much larger than our modelSize in DES, even without any rotator.

I agree that there is no reason in principle.

In practice it is nice to have odd for some applications. For example, odd means the model can be centered on that center pixel, and then convolutions with the PSF image will not cause a shift.

Hmm, stampSize is enforced to be odd upstream as well, so even if we drop that requirement here, you can be guaranteed that stampSize will be odd. Dropping that requirement upstream will have to get discussed within the Science Pipelines team but I can go with whatever is the consensus is on modelSize. But based on Mike's comment, may be we should not make it a requirement just yet.

We want it for shear

In practice it is nice to have odd for some applications. For example, odd means the model can be centered on that center pixel, and then convolutions with the PSF image will not cause a shift.

This consideration is orthogonal to the reasons for having the model use an odd size. You can draw it onto an odd-sized image (and center it) regardless of what the underlying model uses.

OK.

I thought that re-gridding the model from a center-near-pixel-boundary to center-at-pixel-center would introduce larger interpolation errors. If that's not the case then I withdraw my assertion

I'll be moving around the code a bit after merging, so I will drop the requirement then.

I still think people should tend to use odd modelSize in most cases for other reasons. Mostly that typical PSFs have a hot spot right at the center, which an odd model size is better at describing. But making it a requirement feels a little heavy handed. E.g. modeling very out of focus stars, which don't have that central peak, might work better with an even model size for some reason.

arunkannawadi · 2024-04-12T16:04:26Z

Looks like we've converged on the various conversations here. I'll give it a few more hours and merge it by EOD.

arunkannawadi · 2024-04-12T19:32:58Z

Continued in #25 .

esheldon added 2 commits April 4, 2024 13:57

arunkannawadi changed the base branch from main to tickets/DM-43490 April 4, 2024 18:15

use property list to convert to galsim wcs

6d16110

Will work for FITS wcs

revert to using CelestialWcsWrapper

cb3fe08

We think its an issue with the underlying galsim code rather than the wrapper

arunkannawadi reviewed Apr 5, 2024

View reviewed changes

esheldon commented Apr 10, 2024

View reviewed changes

python/lsst/meas/extensions/piff/piffPsfDeterminer.py Show resolved Hide resolved

arunkannawadi reviewed Apr 11, 2024

View reviewed changes

python/lsst/meas/extensions/piff/piffPsfDeterminer.py Show resolved Hide resolved

add check that stampSize/modelSize are odd

5529420

arunkannawadi approved these changes Apr 11, 2024

View reviewed changes

arunkannawadi reviewed Apr 12, 2024

View reviewed changes

rmjarvis reviewed Apr 12, 2024

View reviewed changes

arunkannawadi merged commit 067caea into lsst:tickets/DM-43490 Apr 12, 2024
2 checks passed

esheldon deleted the model-size branch April 18, 2024 17:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add modelSize to piff config #23

Add modelSize to piff config #23

esheldon commented Apr 4, 2024

esheldon commented Apr 5, 2024

esheldon commented Apr 5, 2024

arunkannawadi commented Apr 5, 2024

arunkannawadi Apr 4, 2024

arunkannawadi commented Apr 12, 2024

esheldon commented Apr 12, 2024

arunkannawadi Apr 12, 2024

esheldon Apr 12, 2024

rmjarvis Apr 12, 2024 •

edited

Loading

esheldon Apr 12, 2024

arunkannawadi Apr 12, 2024

rmjarvis Apr 12, 2024

arunkannawadi Apr 12, 2024

rmjarvis Apr 12, 2024

rmjarvis Apr 12, 2024 •

edited

Loading

esheldon Apr 12, 2024

arunkannawadi Apr 12, 2024 •

edited

Loading

esheldon Apr 12, 2024

rmjarvis Apr 12, 2024 •

edited

Loading

esheldon Apr 12, 2024

arunkannawadi Apr 12, 2024

rmjarvis Apr 12, 2024

arunkannawadi commented Apr 12, 2024

arunkannawadi commented Apr 12, 2024

Add modelSize to piff config #23

Add modelSize to piff config #23

Conversation

esheldon commented Apr 4, 2024

esheldon commented Apr 5, 2024

esheldon commented Apr 5, 2024

arunkannawadi commented Apr 5, 2024

Choose a reason for hiding this comment

arunkannawadi commented Apr 12, 2024

esheldon commented Apr 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmjarvis Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmjarvis Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmjarvis Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkannawadi commented Apr 12, 2024

arunkannawadi commented Apr 12, 2024

rmjarvis Apr 12, 2024 •

edited

Loading

rmjarvis Apr 12, 2024 •

edited

Loading

arunkannawadi Apr 12, 2024 •

edited

Loading

rmjarvis Apr 12, 2024 •

edited

Loading