Add cube background model class - #299

#292 (comment)
Ok, I moved the WCS functions to gammapy.utils.wcs. I named it wcs instead of the suggested wcsutils because being it already in utils makes the name redundant. In addition, none of the modules in utils are called somethingutils.

#292 (comment)
You're right: the data members are already Quantity (or Angle) objects. But here's the thing.
This is the trick for storing the data in the FITS table: I need an array of an array, so that the table has only one line. I can either use Quantity, or np.array. The latter won't store the units, so I'd like to keep the Quantity, I think it's the most elegant and simple solution in order to store the data in the desired format, with the units that are already in the data members. Otherwise I'll need the lines that you wisely detected as not needed here:
#292 (comment)

#292 (comment)
Yes, the bg models (in FITS files) from @mimayer do have a E_THRES header keyword defined. In his scripts, he defines it as the energy of the lowest boundary of the 1st energy bin (as I defined them). I followed his workflow there. I don't think an extra data member is needed for it, but I don't have a strong preference: if you still want to have it, please let me know.

#292 (comment)
Ok, the update method worked! Thanks for the tip.

#292 (comment)
OK, but it might get confusing if we call both formats (astropy table and HDU bin table) the same (table).

#292 (comment)
I have to strip the units and put them back on later: np.log10 won't accept a quantity with units.
I leave it as it is, and put a TODO, that when PR #290 is done, it should be reviewed.

#292 (comment)
#292 (comment)

Ok, we discussed this issue already here:

#292 (comment)
#292 (comment)

and I already simplified the methods. They are still long because I add 2 checks for a valid entry from the user. The method itself to find the bin is 4 lines on each case (X, Y and E). I'm not sure I can simplify it more. (We could drop the checks, if you think they don't belong there.)

In the case of the det bin (X, Y), I don't like using the wcs object since it's not a data member (we already discusse this: it makes more sense that the bins are data members instead of the wcs object).

In the case of the energy, I'm not sure what PR #290 does, but one can rewrite it once it is finished, if it makes sense. As I said, finding the bin is only 4 lines: actually it's 2 lines, plus extra 2 lines to find the edges (which I find nice/handy to have).

#292 (comment)
OK, I removed the print statement from here (the message can be added to the exception, as you say).

You say it is no good practice to add print statements to the library code. I think sometimes it can be useful to have some printouts, especially if the code takes long to run (which is not the case), but also to get some important information. An example would be:
https://github.com/gammapy/gammapy/pull/292/files#diff-82cdfbacfcc0119d2ccf6621da08de7bR719
The user requests the plot for a particular energy, and it gets to know the boundaries of the slice used for the plot. This is maybe not the most useful case, but serves as an example for my question: how would you issue such an information?

I removed in any case all print statements of the class.

About the plots:

#292 (comment)
I do need it for the color axis. So it stays.

#292 (comment)
#292 (comment)
I removed the save options.

#292 (comment)
OK, I was using for instance the image object to get the data in the tests of the plotting functions. But I learnt how to get them from the ax object, so no need for them. Now the functions return only the ax object.

#292 (comment)
I know. Ok, I adopted your naming, though I find more explicative the old denomination of the functions.

OK I just pushed a commit addressing all open comments in the old PR #292. I commented above on some of them (the non-trivial ones). Please have a look at my replies, and if applicable please answer to them on a new comment.

For future comments, please use this PR (#299).

Permanent link to the docs:
https://googledrive.com/host/0BzXHZQx0oCLBfmcyUDdxMU9sLVduMXM1QXk5RTNYU3dXYmthVG9OZ18wWE1uR3lYaEsxRU0/html_doc/docs/_build/html

@mapazarr – I'm short on time this week, so I'll just do a final review tomorrow during the hangout.

Please update the task list and ask if you have any specific questions before you can finish this up.

Concerning the E_THRES header keyword ... if it's set to the energy array lower edge it's duplicated info and useless. But maybe in the future we do want to set it to indicate a valid range, or Gammalib requires it, so OK to keep if you like.

Ok, I'm pretty much done with this PR. I think I have still one or 2 minor TODO's for you to review/comment on, on top of the questions I posted acouple of days ago above, and of course the final code review.

The link to the docs is here:
https://googledrive.com/host/0BzXHZQx0oCLBfmcyUDdxMU9sLVduMXM1QXk5RTNYU3dXYmthVG9OZ18wWE1uR3lYaEsxRU0/html_doc/docs/_build/html

This example doesn't work for me:

$ python examples/plot_background_model.py 
Traceback (most recent call last):
  File "examples/plot_background_model.py", line 23, in <module>
    write_kwargs=dict(clobber=True))
  File "/Users/deil/code/gammapy/gammapy/background/models.py", line 475, in write
    self.to_fits_image().writeto(outfile, **write_kwargs)
  File "/Users/deil/code/gammapy/gammapy/background/models.py", line 432, in to_fits_image
    imhdu.header.update(wcs_header)
  File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/io/fits/header.py", line 1167, in update
    update_from_dict(k, other[k])
  File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/io/fits/header.py", line 1149, in update_from_dict
    card = Card(k, v)
  File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/io/fits/card.py", line 448, in __init__
    self.value = value
  File "/Users/deil/Library/Python/3.4/lib/python/site-packages/astropy-1.1.dev12994-py3.4-macosx-10.10-x86_64.egg/astropy/io/fits/card.py", line 570, in value
    raise ValueError('Illegal value: %r.' % value)
ValueError: Illegal value: .

The problem are the '' and 'COMMENT' entries:

In [1]: dict(wcs_header)
Out[1]: 
{'': ,
 'CDELT1': 0.546914274042,
 'CDELT2': 0.546914274042,
 'COMMENT':  WCS header keyrecords produced by WCSLIB 5.8,
 'CRPIX1': 0.5,
 'CRPIX2': 0.5,
 'CRVAL1': -6.01605701447,
 'CRVAL2': -6.01605701447,
 'CTYPE1': 'DETX',
 'CTYPE2': 'DETY',
 'CUNIT1': 'deg',
 'CUNIT2': 'deg',
 'LATPOLE': 90.0,
 'WCSAXES': 2}

Merging the headers like this works for me, so that's what I'd suggest as a workaround:

header = wcs_header.update(imhdu.header)

The reason this error doesn't show up for you locally and on travis-ci is probably that you're not using the very latest Astropy master.
I'll make an issue that travis-ci docs builds with Python 3 and with the latest astropy master should be added.

Left some more minor inline comments.

Ok I implemented the last comments. I also fixed some problems in SpectralCube nad the tutorials, that where preventing the sphinx job in travis-ci to work.

Can we merge this PR already? Some of the minor comments are turning around on issues discussed a few times already, and undoing some of the proposed changes.

Merging this now.
Thanks for fixing all the issues that came up in this PR or following up in other Astropy / Gammapy issues!

I looked through the coverage report

time python setup.py test --remote -V --coverage
open htmlcov/gammapy_background_models.html

and generally coverage is good.
The only thing that's not tested is the code under except ValueError in _parse_bg_units
Why is that needed at all? Maybe because the background cubes from Michael or Gammalib don't have correct units per the FITS standard? If so we should tell them to fix it and make a note that this unit handling helper function should be removed as soon as it's done.

🎆

Docs build on readthedocs worked: https://gammapy.readthedocs.org/en/latest/background/models.html

Hi Christoph. Indeed the existence of _parse_bg_units is because of the units in @mimayer's files. I checked and they actually comply the FITS standard, but they are not understood by ~astropy.units.Units.

Please see the text at the examples section of: https://heasarc.gsfc.nasa.gov/docs/heasarc/ofwg/docs/general/ogip_93_001/ogip_93_001.html

'the number of spaces between individual unit sub-strings is left a matter for personal preference'

@mimayer doesn't use any spaces, i.e. 1/s/MeV/sr, while Quantity expects spaces and can't parse this unit, i.e. 1 / s / MeV / sr.

According to the FITS standards both are valid.

This could be another issue for astropy.

Thanks for the merging! :-)

I didn't get any coveralls report.

You have to manually go to https://coveralls.io/github/gammapy/gammapy (I always do it via the link here) and find the right build.
There is the possibility to configure Coveralls to post comments on PRs with links to the right build.
But so far we weren't using that much and it was annoying.

BTW, I think the test file from gammalib also was having the issue with the units.