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

Fix IACTBasicImageEstimator psf method #1294

Merged
merged 5 commits into from Feb 12, 2018

Conversation

Projects
None yet
2 participants
@gallanty
Contributor

gallanty commented Feb 8, 2018

Changed psf() method to return image with odd number of pixels and
PSF centered in the middle of the central pixel.
PSF image FOV set to 99% containment radius.
PSF sky position is that for which the PSF is computed (center of
the reference image by default for now).

gallanty added some commits Feb 8, 2018

Changed psf() method to return image with odd number of pixels and
PSF centered in the middle of the central pixel.
PSF image FOV set to 99% containment radius.
PSF sky position is that for which the PSF is computed (center of
the reference image by default for now).

@cdeil cdeil self-assigned this Feb 9, 2018

@cdeil cdeil added the bug label Feb 9, 2018

@cdeil cdeil added this to the 0.7 milestone Feb 9, 2018

@cdeil cdeil changed the title from Fix of IACTBasicImageEstimator psf() method to Fix IACTBasicImageEstimator psf method Feb 9, 2018

@cdeil

Looks like your current code doesn't fill the kernel data, basically you get random uninitialised bytes back from the np.empty call, no?

I would suggest you call this to compute the kernel array:
http://docs.gammapy.org/dev/api/gammapy.irf.TablePSF.html#gammapy.irf.TablePSF.kernel
And then wrap this in a SkyImage and add a WCS as you do now.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 9, 2018

I'd suggest this way to set rad_max:

        if rad_max is None:
            rad_max = psf_mean.containment_radius(containment_fraction)
        else:
            rad_max = Angle(rad_max)
@cdeil

This comment has been minimized.

Member

cdeil commented Feb 9, 2018

@gallanty - Implementation looks good to me now. Let me know when you're done here and then I'll merge.

erange = u.Quantity((p['emin'], p['emax']))
psf_mean = mean_psf.table_psf_in_energy_band(erange, spectrum=self.spectral_model)
radmax = psf_mean.containment_radius(0.99)

This comment has been minimized.

@cdeil

cdeil Feb 9, 2018

Member

Maybe expose this containment fraction as a keyword argument with default 0.99 on this method?

Then the default behaviour is the same, but the caller has control over this. (for many PSFs the tail is noisy and 99% containment is not well defined or the numerical method to compute the 99% containment radius might even fail.
For that reason, we changed to rad_max in http://docs.gammapy.org/dev/api/gammapy.irf.TablePSF.html#gammapy.irf.TablePSF.kernel
and maybe the same change of leaving this up to the caller might be a good idea here?

@gallanty

This comment has been minimized.

Contributor

gallanty commented Feb 11, 2018

SkyImage.data now filled directly with TablePSF.kernel() output.
[Was previously filled via TablePSF.evaluate() method using pre-existing code, there was no bug. Latest result differs from that earlier one by less than 1% - but I agree it is cleaner this way.]
Implemented user-accessible containment_fraction parameter to psf() method, as well as overriding rad_max parameter, as you suggested Friday. Added a few lines to the docstring which should document this.

@cdeil

This comment has been minimized.

Member

cdeil commented Feb 12, 2018

@gallanty - Looks good to me. Thanks!

@cdeil

cdeil approved these changes Feb 12, 2018

@cdeil cdeil merged commit bc69ca8 into gammapy:master Feb 12, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment