GalSim-developers / GalSim

The modular galaxy image simulation toolkit. Documentation:
http://galsim-developers.github.io/GalSim/
Other
224 stars 106 forks source link

Implement Spergel (2010) galaxy profile model #616

Closed mdschneider closed 9 years ago

mdschneider commented 9 years ago

Motivation: We are interested in speeding up the rendering of galaxy images for use in MCMC algorithms to infer galaxy model parameters given a postage stamp image. An alternative galaxy model parameterization may be helpful.

The slowest part of the prediction of a galaxy image is the convolution with the PSF. Here's a plan for moving the PSF convolution into a pre-computation step, leaving MCMC steps to a simple linear operation for galaxy image predictions,

While GalSim predictions can be sped up by changing GSParams settings, we would like to avoid identifying the combination of accuracy and speed tradeoffs for each application if a re-parameterization can be made to work.

Credit for benchmarks and planning: @jmeyers314

rmandelb commented 9 years ago

What kind of interplay between C++ and python layers do you anticipate needing for the second part of this (the PSF Zernike expansion)? Right now, all Zernike-related calculations happen in the python layer, so it seems like what you're describing could require substantial refactoring of the code in order to be feasible?

mdschneider commented 9 years ago

The chief requirement I am assuming above is that the PSF can be expanded in a series in such a way that any PSF model parameters are confined to the linear coefficients of the series. Then the terms with spatial dependence in the series can be individually convolved with the terms in the series expansion of the galaxy model.

By assuming a Zernike expansion of the exit pupil wavefront, it is possible to explicitly derive the series expansion of the PSF that has as coefficients the wavefront Zernike coefficients. See, for example, equations (7) and (8) here.

This approach will require new GSObjects (and corresponding SBProfiles) representing each term in the series expansion of the PSF. These objects are intended to be passed to the Convolve method along with the terms in the series expansion of the galaxy profile. So, I don't think we will need significant code refactoring. However, we may not directly utilize the existing Zernike wavefront methods (but these should be very important for validation).

This approach might work for wavefronts that are not well described by a low-order Zernike expansion. But I have only understood how a finite series might be useful for the case of a low-order Zernike expansion of the wavefront. So, crucially, this is not an approach for pre-computing atmosphere PSF convolutions.

mdschneider commented 9 years ago

Branch started for this issue: c2b9afa4a30c6ed382e2aec9e9eb9265adf820f7

rmandelb commented 9 years ago

Michael - I didn't get to fully read through and grok your message, but since you're already coding, I wanted to mention something you might find useful. If you follow the instructions in devel/commit-msg, then git will automatically append the branch name to each commit message for you, which means that the commits will show up on this issue page automatically (after you push them). That can be really useful for later discussions of the code that might happen on this issue.

jmeyers314 commented 9 years ago

@mdschneider, I added a unit test for generating a few circularly symmetric Spergel profiles of varying indices. They await your completion of a "half_light_radius" input keyword.

jmeyers314 commented 9 years ago

I'm guessing we can ignore the info cacheing here since the Spergel profile is analytic both in real space and Fourier space: no need to cache the results of Hankel transforms and such.

rmjarvis commented 9 years ago

You might still want the Info class even without the need for caching the Hankel transform. cf. SBExponential, which mostly uses its Info class to cache the calculations needed for photon shooting.

But if that is analytic too, then you can skip all the Info stuff here.

jmeyers314 commented 9 years ago

So I think I did more harm than good in b0eadfe. Looking for some git-fu: anyone know how to excise just the one commit from this branch?

-Josh

rmjarvis commented 9 years ago

The simple revert function: git revert b0eadfe should be fine in your case. BTW, when things get more complicated, this is a useful site to check out:

http://sethrobertson.github.io/GitFixUm/fixup.html

But I believe your "adventure" ends with git revert.

jmeyers314 commented 9 years ago

I think I got Fourier mode drawing basically working for this now. GalSim output matches my straight python test script. A couple of interesting features popped up:

Next step, photon-shooting.

rmandelb commented 9 years ago

It is certainly the case that we've run into issues when people try to draw deVauc profiles without a convolution, and our standard solution is to suggest convolving with at least a small PSF. So I'm guessing that fix should work for you. It sounds like something to mention in the docstring.

jmeyers314 commented 9 years ago

I think the first part of this issue (drawing the Spergel profile without any Taylor expansion trickery) is almost ready for a PR. The last remaining question I have is what to do for photon-shooting when nu<0 (in which case the s.b. profile diverges at the origin). I think this should be possible in principle, but in practice the rejection sampling near the origin ends up in an infinite loop waiting for a deviate smaller than 1/inf. Any suggestions?

Some other items of note:

rmjarvis commented 9 years ago

in practice the rejection sampling near the origin ends up in an infinite loop waiting for a deviate smaller than 1/inf. Any suggestions?

Where exactly is the problem happening? Making the OneDimensionalDeviate? Or when drawing from the interval (Interval::drawWithin)?

Anyway, some things you could try:

I noticed a couple weeks ago that the Spergel profile is the F.T. of the Moffat profile. Not sure how to take advantage of that but I thought it was interesting. :)

Aside from using the analytic form in kValue, probably not much else to do. The Moffat class already uses this in reverse. We used to use a tabulated FT, before I discovered the analytic version -- it was a significant speed up to switch to using cyl_bessel_k.

jmeyers314 commented 9 years ago

Where exactly is the problem happening? Making the OneDimensionalDeviate? Or when drawing from the interval (Interval::drawWithin)?

Problem was in Interval::drawWithin. I went ahead and implemented your fancy suggestion above, which turned out to not be too difficult. Seems to work fine now.

rmandelb commented 9 years ago

@jmeyers314 or @mdschneider , should this issue be closed? Or did you have a further set of work to be done on this issue after the initial PR?

jmeyers314 commented 9 years ago

@jmeyers314 or @mdschneider , should this issue be closed? Or did you have a further set of work to be done on this issue after the initial PR?

I had originally envisioned keeping this open until #628 is merged, as the original description by @mdschneider included the series implementation of the Spergel and PSF profiles. Maybe it's sufficient to just keep 628 open though.

rmandelb commented 9 years ago

It's up to you, really, but it seems to me that having two open issues for basically the same body of work isn't necessary.

jmeyers314 commented 9 years ago

I guess that's fair. I'm certainly not working in branch 616 anymore. Closing.