Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-34531: Make piff PSF model size configurable #9

Closed
wants to merge 3 commits into from

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! You might want to wait for Josh's comment just in case.

default=21,
min=11,
max=35,
inclusiveMax=True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a check= option to ensure it's always odd?

@jmeyers314
Copy link
Collaborator

@parejkoj
Copy link
Contributor Author

Thanks @jmeyers314 : I hadn't noticed the kernelSize in the parent class. It looks like it might be used rather differently there, though, with the min/max values being related to some calculated kernel size, not the one in the config? I wish those were documented better.

@arunkannawadi
Copy link
Member

Josh and I briefly looked into this yesterday, and it looks like the docstring for kernelSize in the base class is incorrect. It reads like it's a measure of size in units of measured sizes, but it really is just the number of pixels along a side to draw a PSF image. It's therefore consistent with the use here.

As for using a RangeField, there's no algorithmic reason to restrict it between 11 and 35, and would rather let that be configurable as well. One thing we could do is to throw an error in validate method if kernelSize exceeds kernelSizeMin and kernelSizeMax. Currently, it is being clipped and I'd rather throw an error instead doing something else silently.

@parejkoj
Copy link
Contributor Author

Except that, if I'm reading the pcaPsf and psfexPsf classes correctly, kernelSizeMin and kernelSizeMax are used to check whether the kernel size, after scaling by the stellar quadrupole moment, is within some range. So, I don't think those are even relevant to piff at all, since it doesn't do any such scaling. psfexPsf even redefines config.kernelSize with a new docstring an a default of 81.

So, I think we want to revert most of these changes to piff here, but maybe add some clarifying comments to config.setDefaults about how kernelSize is used, and probably drop the usage of min/max entirely?

@jmeyers314
Copy link
Collaborator

Except that, if I'm reading the pcaPsf and psfexPsf classes correctly, kernelSizeMin and kernelSizeMax are used to check whether the kernel size, after scaling by the stellar quadrupole moment, is within some range.

I think you're right in principle. In practice, the kernelSize we've been using for HSC at least is 81!

grep kernelSize /repo/main/HSC/runs/RC2/w_2022_08/DM-33741/20220217T181232Z/characterizeImage_config/*.py

gives

... snip ...
config.measurePsf.psfDeterminer['psfex'].kernelSize=81
... snip ...

and is outside the nominal min/max. I don't know about DC2 or other datasets. I suspect we may not have been using the quadrupole scaled stuff in a while, which is why I didn't think to implement that for piff.

@yalsayyad
Copy link

yalsayyad commented Apr 29, 2022

Hi! Was directed here by @parejkoj re my question for what default configs to choose.

@jmeyers314 So what does it mean when you say that the psfDeterminer's kernelSize for HSC is 81, when the actual output PSF model kernels are 41 pixels (e.g. the ones you'd get by...

calexp =  butler.get('calexp', visit=30504, detector=54)
calexp.getPsf().computeKernelImage(calexp.getBBox().getCenter()).getBBox()
>>> Box2I(corner=Point2I(-20, -20), dimensions=Extent2I(41, 41))

It says 21 pixels for the post-PIFF butler = Butler('/repo/main', collections=['HSC/runs/RC2/w_2022_16/DM-34451'])

Leave the default as 21 for further testing.
Explicitly test it drawSize.
Use drawSize in validatePsfCandidates.

fixup
kernelSize from the BasePsfDeterminerConfig has a quite different
meaning in the pca and psfex determiners.
@arunkannawadi
Copy link
Member

what does it mean when you say that the psfDeterminer's kernelSize for HSC is 81, when the actual output PSF model kernels are 41 pixels

So for psfex, the kernelSize is set to 81 and samplingSize to 0.5. Since 81*0.5 ~ 41, it takes a 41x41 image around the stars to determine the PSF and generates an oversampled 81x81 image when asked to draw a PSF model.

@arunkannawadi arunkannawadi marked this pull request as draft June 3, 2022 02:44
@arunkannawadi
Copy link
Member

Closing this PR without merging, since this is no longer relevant. This has been taken care in DM-36071 and this ticket has been marked as Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants