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 gaussian psf to AtmosphericPSF #197

Merged
merged 8 commits into from
Feb 4, 2019
Merged

Add gaussian psf to AtmosphericPSF #197

merged 8 commits into from
Feb 4, 2019

Conversation

jmeyers314
Copy link
Member

This PR follows from the discussion on slack channel #desc-dm-dc2 around Jan 9-11, 2019. It adds an option to convolve in an additional Gaussian PSF to the AtmosphericPSF model to account for otherwise unmodeled effects, such as dome seeing, polishing errors, vibration, and other errors listed in document LTS-124.

I also took the opportunity to switch from using fsolve to invert the Tokovinin fitting formula for r0_500 to bisect, which I've found to be more robust. I also changed the code to target the seeing at the specific filter effective wavelength being drawn instead of the seeing at 500 nm. These two changes are mostly aesthetic, yielding delivered FWHM differences around ~0.01 arcseconds.

The latter seems to be more robust than the former.
... instead of the FWHM at 500nm.
This is to represent unmodeled additional contributions to the PSF.
The default value of 0.3 arcsec FWHM comes from the 0.4 arcsec
represented in LTS-124, minus (in quadrature) the modeled contributions
of 0.26 arcsec for diffusion, 0.1 arcsec for the optics design, and
0.09 arcsec for for residual misalignments, and then rounded up from
0.27 to 0.3.
@coveralls
Copy link

coveralls commented Jan 12, 2019

Coverage Status

Coverage increased (+0.1%) to 73.417% when pulling 6f2c2de on addGaussianPSF into 037a8e3 on master.

Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

lgtm. I'm running this on the Run2.1i visits and the resulting fwhm numbers are consistent with expectations.

@@ -200,4 +228,9 @@ def _getPSF(self, xPupil=None, yPupil=None, gsparams=None):
t0=self.t0,
exptime=self.exptime,
gsparams=gsparams)
if self.gaussianFWHM > 0.0:
psf = galsim.Convolve(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jmeyers314 @rmjarvis Unfortunately, doing this convolution breaks this code: https://github.com/lsst/sims_GalSimInterface/blob/master/python/lsst/sims/GalSimInterface/galSimInterpreter.py#L1006
Could you guys have a look please?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry. That section was rather hacky, depending on some implementation details that I guess are now being changed. Probably a sign of bad design...

But in the interest of just getting this working, rather than refactoring that part of the code, I think you just need to swap the order of the items in this Convolve. Put the Gaussian first, and the current psf second.

If that doesn't work, please post (here if you want, or on Slack if that's easier) the exact error message that you are getting from the fft section.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But in the interest of just getting this working, rather than refactoring that part of the code, I think you just need to swap the order of the items in this Convolve. Put the Gaussian first, and the current psf second.

That's the first thing I tried, but the last item in obj.original.obj_list is still a Convolution object...obj.original.obj_list[-1].obj_list[-1] would be the PhaseScreenPSF that we'd want to replace, but that's only if gaussianFWHM > 0. I can see increasingly hacky ways of getting this to work, but I thought you guys might want to consider some design changes.

In any case, here's the error I see:

Traceback (most recent call last):
  File "/global/cscratch1/sd/jchiang8/desc/dev/imSim/bin/imsim.py", line 91, in <module>
    image_simulator.run(processes=args.processes)
  File "/global/cscratch1/sd/jchiang8/desc/dev/imSim/python/desc/imsim/ImageSimulator.py", line 253, in run
    results.append(simulate_sensor(self.gs_obj_dict[det_name]))
  File "/global/cscratch1/sd/jchiang8/desc/dev/imSim/python/desc/imsim/ImageSimulator.py", line 392, in __call__
    fft_sb_thresh=fft_sb_thresh)
  File "/global/cscratch1/sd/jchiang8/desc/dev/sims_GalSimInterface/python/lsst/sims/GalSimInterface/galSimInterpreter.py", line 940, in drawObject
    gain=detector.photParams.gain)
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/gsobject.py", line 1585, in drawImage
    self._prepareDraw()
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/transform.py", line 274, in _prepareDraw
    self._original._prepareDraw()
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/convolve.py", line 285, in _prepareDraw
    obj._prepareDraw()
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/convolve.py", line 285, in _prepareDraw
    obj._prepareDraw()
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/phase_psf.py", line 1397, in _prepareDraw
    self._screen_list._prepareDraw()
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/phase_psf.py", line 868, in _prepareDraw
    psf._step()
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/phase_psf.py", line 1406, in _step
    wf = self._screen_list._wavefront(u, v, None, self.theta)
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/phase_psf.py", line 927, in _wavefront
    return np.sum([layer._wavefront(u, v, t, theta) for layer in self], axis=0)
  File "/global/cscratch1/sd/jchiang8/desc/dev/galsim_local/lib/python3.6/site-packages/galsim/phase_psf.py", line 927, in <listcomp>
    return np.sum([layer._wavefront(u, v, t, theta) for layer in self], axis=0)
AttributeError: 'OptWF' object has no attribute '_wavefront'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. I forgot we got rid of a feature we used to have where if any argument to a Convolve was itself a Convolution, it would combine the lists. I guess we could do that manually in PSFbase.applyPSF:

if isinstance(psf, galsim.Convolution):
    return galsim.Convolution([obj] + psf.obj_list)
else:
    return galsim.Convolution(obj, psf)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, making that change and swapping the order of the items in the Convolve in atmPSF._getPSF addresses the issue. I'll go ahead and make the PR in sims_GalSimInterface.

@jchiang87
Copy link
Collaborator

@jmeyers314 @cwwalter I think this is ready to merge. I added some code to read the gaussianFWHM value for the additional Gaussian to convolve from the config file. I added a unit test to ensure that these changes are working as expected, but if you guys want to have a look, please do so. Unless I hear otherwise, I'll go ahead and merge this into master Monday morning.

@jchiang87 jchiang87 merged commit aa0f896 into master Feb 4, 2019
@jchiang87 jchiang87 deleted the addGaussianPSF branch February 4, 2019 16:50
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