Development in gammapy.maps

[woodmd]

This PR contains changes related to ongoing development in gammapy.maps. All of the code here is in an experimental/development state so it's possible that method and class names may change in the future. The intention was to give everyone an opportunity to review and give feedback on the current class/interface design. A quick summary of the updates included here:

• Fills out more of the class hierarchy. WCS and HPX maps now both inherit from a MapBase class that defines abstract methods for accessing the contents of a map. My plan is to have two concrete instances each for WCS and HPX -- one for non-sparse maps (HpxMapND and WcsMapND) and one for sparse maps (HpxMapSparse and WcsMapSparse). Both can represent maps of arbitrary dimensionality (i.e. 2 +N).
• Implemented I/O routines for HpxMapND. These support reading/writing maps to FITS according to the format conventions documented here. There are still a few aspects of the format that need to be worked out -- mostly pertaining to how axes can be extracted from the BANDS table in an automated way.
• Added stubs for some of the WCS classes mainly for testing the hpx to wcs conversion methods.

There's obviously still a lot of work to be done but I plan to continue working on this over the next few months. Once I'm satisfied that these classes have everything we need in fermipy I would start working on copying over functionality from SkyImage and SkyCube.

[cdeil]

@woodmd - I don't have time for a detailed review before going on vacation.

Here's my suggestions based on a quick look:

Locally with

python -m pytest -v gammapy/maps

I see this fail (because you're passing shape=[3, array([768])] to np.random.uniform(0, 1, shape) in test_hpxcube_init:

gammapy/maps/tests/test_hpxcube.py::test_hpxcube_init[8-False-GAL-None-None] FAILED
gammapy/maps/tests/test_hpxcube.py::test_hpxcube_init[8-False-GAL-None-axes1] FAILED
gammapy/maps/tests/test_hpxcube.py::test_hpxcube_init[8-False-GAL-DISK(110.,75.,10.)-axes2] FAILED
gammapy/maps/tests/test_hpxcube.py::test_hpxcube_init[8-False-GAL-DISK(110.,75.,10.)-axes3] FAILED

Please fix.

In addition I see some warnings about unclosed files from the tests. There my suggestion would be to use with fits.open, but since this might be controversial, I've made that in #1055 .

I noticed that tests for gammapy.maps don't run on travis-ci. They should start here, after the gammapy.irf tests:
https://travis-ci.org/gammapy/gammapy/jobs/238624098#L1654
The reason is that healpy isn't installed in .travis.yml. I'm making a separate PR now to change this.

I would also suggest to start to put gammapy.maps in the docs. Only then people will start to use it and give real-world feedback. Developing it will take time, and we want user feedback. I think we can just put a warning like
https://github.com/gammapy/gammapy/blob/master/gammapy/maps/__init__.py#L4
at the start of the gammapy.maps docs making it clear that we don't promise API stability here. It's the case everywhere in Gammapy now, as long as we're still pre 1.0, but here especially we can add the warning.

Otherwise, opening up the gammapy.maps code up in PyCharm and reading through it a bit, some obvious bugs are pointed out. Just to name one example that also pyflakes gammapy/maps shows:

gammapy/maps/hpxmap.py:233: undefined name 'ew_hpx'

Another one is the MapBase.data setter, which sets _counts, but I think this was renamed to _data in __init__, i.e. is broken?

    @data.setter
    def data(self, val):
        if val.shape != self.data.shape:
            raise Exception('Wrong shape.')
        self._counts = val

There's also a remaining import of fermipy from here:

gammapy/maps/hpx.py
175:    hpx : `~fermipy.hpx_utils.HPX`
1177:        from fermipy.skymap import Map
1200:        from fermipy.skymap import Map

@woodmd - Is it useful if I point out such things?
I can try to fix some of them, but often I don't know which methods of the classes that are there now should be fixed or can be completely removed.
I find it hard to do a review / feedback based on the diff on Github because I'm not very familiar with the code, and would find it easier to approach gammapy.maps from the point of trying to increase test coverage and remove or fix the parts that aren't needed or completely broken first.

[woodmd]

I see this fail (because you're passing shape=[3, array([768])] to np.random.uniform(0, 1, shape) in test_hpxcube_init:

Ok I'll have a look. I wasn't getting this error when I ran unit tests locally so it must be something to do with python3 or version of numpy.

In addition I see some warnings about unclosed files from the tests. There my suggestion would be to use with fits.open, but since this might be controversial, I've made that in #1055 .

I don't have a strong feeling about this so I'm happy to adopt whatever convention you use elsewhere.

I would also suggest to start to put gammapy.maps in the docs.

I fully agree. If you create a page in the appropriate place of the docs hierarchy I can start writing documentation there.

Otherwise, opening up the gammapy.maps code up in PyCharm and reading through it a bit, some obvious bugs are pointed out.

Yes I'm aware of these. Most of these arise from code that I've copied from fermipy but haven't yet written unit tests for so I wouldn't really spend time on these (I can run pyflakes just as easily). What I was mainly looking for feedback on was the overall design with respect to class hierarchy and interfaces.

[cdeil]

@woodmd - With #1063 merged tests here should now run when you push a new commit.

• Yes, I would suggest we start using with fits.open(...) as hdu_list in Gammapy. Please do it here and I'll do it for all other parts of Gammapy except gammapy.maps soon.
• For the docs, all you have to do is to add docs/maps/index.rst and a line maps/index to the toctree directive here: https://github.com/gammapy/gammapy/blame/master/docs/index.rst#L55 Concerning the structure of the high-level docs page, you could copy & paste e.g. docs/catalog/index.rst and keep the main sections / formatting, but remove the content to start. Most of the Gammapy high-level docs need to be re-done, but the idea is to follow the Astropy pattern and to have docs for each sub-package and there have the structure "Introduction", "Getting started", then optionally more content or links to other pages and at the end "Reference/API". Should I put an empty docs/maps/index.rst in-place in a separate PR, or will you just add it here?

[woodmd]

Thanks for adding healpy to the travis builds. I'll take care of making a commit to this PR adding a placeholder for the documentation.

[woodmd]

The remaining errors appear to be unrelated to this PR. Merging now.