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

Improve SkyCube convolution and npred computation #766

Merged
merged 7 commits into from Nov 11, 2016

Conversation

@adonath
Copy link
Member

@adonath adonath commented Nov 10, 2016

This is the third part of the major SkyCube rework. These are the most important changes in this PR:

  • Move stand-alone convolve_cube function to SkyCube.convolve()
  • Move stand-alone cube_to_spec function to SkyCube.to_spectrum, but currently not working.
  • Add a EnergyDependentTablePSF.kernels() method to get a set of kernels for cube convolution
  • Rewrite compute_npred_cube function

When running the script docs/tutorials/npred/npred_convolved.py , I get very good agreement with the result from the fermi science tools. The total number of counts agrees within >1%, depending on the precision of the flux image integration.

@adonath adonath self-assigned this Nov 10, 2016
@cdeil cdeil added the feature label Nov 10, 2016
@cdeil cdeil added this to the 0.5 milestone Nov 10, 2016
Copy link
Member

@cdeil cdeil left a comment

I left a few inline comments.
Overall the changes look good, but are too large for me to review in detail this week.

So thank you very much for this cleanup and new features. It's great that you pushed on this and now get agreement with the Fermi ST!

energy_axis = LogEnergyAxis(energy, mode='edges')
elif mode == 'center':
energy = Energy.equal_log_spacing(emin, emax, enumbins, eunit)
energy_axis = LogEnergyAxis(energy, mode='center')

This comment has been minimized.

@cdeil

cdeil Nov 10, 2016
Member

Please always end with

else:
    raise ValueError(...)

for multiple-choice based on user string input.
It really helps to get a good error message when you mistype something.

2-dim sky image
"""
data = self.data[idx]
return SkyImage(name=self.name, data=data, wcs=self.wcs)

This comment has been minimized.

@cdeil

cdeil Nov 10, 2016
Member

Return copy for data and wcs, at least by default, to avoid accidentally coupled objects?
(I thought we had that method already, didn't review the SkyCube class again. Just ignore me if this is not a good suggestion)

Parameters
----------
kernels : list
List of 2D convolution kernels.

This comment has been minimized.

@cdeil

cdeil Nov 10, 2016
Member

What are kernels? Numpy arrays? Astropy kernel objects?
Above you say:

Convolve cube with a given energy dependent PSF.
which suggests the input would be such a PSF object, which I guess it isn't?

Can you make this more clear in the docstring or put an example here or in the high-level docs and link to it from here?

Returns
-------
kernels : list
List of 2D image kernels.

This comment has been minimized.

@cdeil

cdeil Nov 10, 2016
Member

Say what type of object the kernels are.

Reference sky cube.
kwargs : dict
Keyword arguments passed to
`EnergyDependentTablePSF.table_psf_in_energy_band()`.

This comment has been minimized.

@cdeil

cdeil Nov 10, 2016
Member

Does this resolve as a Sphinx link in the HTML docs?

@adonath adonath force-pushed the adonath:rework_cube_convolve_npred branch from 4f75afd to 6a1272c Nov 11, 2016
@adonath
Copy link
Member Author

@adonath adonath commented Nov 11, 2016

@cdeil Thanks! I've addressed all your comments and will merge as soon as Travis-CI passed.

@adonath adonath force-pushed the adonath:rework_cube_convolve_npred branch from b317bfe to e7ece72 Nov 11, 2016
@adonath adonath merged commit dd8761a into gammapy:master Nov 11, 2016
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@adonath adonath deleted the adonath:rework_cube_convolve_npred branch Nov 11, 2016
@adonath
Copy link
Member Author

@adonath adonath commented Nov 11, 2016

@cdeil I've merged this PR you can continue with #754.

@cdeil
Copy link
Member

@cdeil cdeil commented Nov 11, 2016

@adonath - Thanks!

I've done some minor code formatting in 290b7ba in master.
Have to go now, but will finish up #754 asap.

@cdeil cdeil changed the title Rework cube convolution and npred computation Improve SkyCube convolution and npred computation Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants