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

Add 2nd central moments to base+compound GSObjects #815

Open
jmeyers314 opened this issue Oct 17, 2016 · 4 comments
Open

Add 2nd central moments to base+compound GSObjects #815

jmeyers314 opened this issue Oct 17, 2016 · 4 comments
Assignees
Labels
desc Of possible interest to LSST DESC members looking for a project feature request Request for a new feature in GalSim starter projects Possibly a good choice for someone looking to get started on GalSim development

Comments

@jmeyers314
Copy link
Member

I think it would be nice to have unweighted second central moment properties for our analytic GSObjects:

obj = galsim.Gaussian(half_light_radius=1)
print(obj.Ixx)  # 0.7205
print(obj.Ixy)  # 0.0

I think it's straightforward to define these for at least the analytic profiles in base.py. It should also be straightforward to propagate these through transformations and sums/convolutions, though it's probably good to avoid InterpolatedImage moments, or using moments as input.

@rmandelb
Copy link
Member

How relevant are the analytic ones, considering that they ignore pixel convolution?

@jmeyers314
Copy link
Member Author

How relevant are the analytic ones, considering that they ignore pixel convolution?

I think we can leave the pixel convolution up to the user:

gal = galsim.Gaussian(half_light_radius=1)
print(gal.Ixx)  # 0.7205
pix = galsim.Pixel(1)
print(pix.Ixx)  # 0.0833  (i.e., 1./12)
obj = galsim.Convolve(gal, pix)
print(obj.Ixx)  # 0.80383 = 0.7205+0.0833

@rmandelb
Copy link
Member

Are you not concerned about the fact that we generally discourage users from explicitly doing the pixel convolution, hiding it under the rug in drawImage with the default image rendering methods, and here to get it right they do have to explicitly do it?

I'm trying to decide what I think about this. I don't love it but I suppose with adequate documentation plus maybe a demonstration in the demos, it's OK.

@rmjarvis
Copy link
Member

I don't have a problem with this. Many of these objects have attributes like fwhm or half_light_radius when those are analytically known, and they don't include a pixel convolution. So it seems consistent for these not to as well.

If you do this, you should remember to update the calculateMomentRadius function to look for these values when they exist and skip the calculation. Right now, it only does so for Gaussian.

@rmjarvis rmjarvis added the feature request Request for a new feature in GalSim label Nov 21, 2016
rmjarvis added a commit that referenced this issue May 14, 2018
@rmjarvis rmjarvis added starter projects Possibly a good choice for someone looking to get started on GalSim development desc Of possible interest to LSST DESC members looking for a project labels Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desc Of possible interest to LSST DESC members looking for a project feature request Request for a new feature in GalSim starter projects Possibly a good choice for someone looking to get started on GalSim development
Projects
None yet
Development

No branches or pull requests

3 participants