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 Fermi Vela dataset #156

Merged
merged 3 commits into from Jul 30, 2014

Conversation

Projects
None yet
2 participants
@ellisowen
Contributor

ellisowen commented Jul 26, 2014

Can these be added? I want to use them for the unit tests & tutorial for npred cube and needed a single clear source to do a good comparison between my npred method (see PR #151) and the fermi science tools (so couldn't just use the FermiGalacticCenter stuff). Introduces:
357.1 kB counts_vela.fits.gz
1.5 kB exposure_vela.fits.gz
Are these an OK size to include? I could probably reduce the resolution + size of the counts image once I've written the tutorial & know what is needed there.

I picked the vela region as I already had these files to hand. Could use a different region if preferred.

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Jul 27, 2014

I want to make a few changes to this - will push updates shortly.

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 28, 2014

The test error on Python 2.6 is a known issue: #147

You have to skip the exposure cube test on Python 2.6 as described here.

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 28, 2014

As discussed, please put the files in the https://github.com/gammapy/gammapy-examples repo (in a data/fermi/vela folder) and only keep the load function and docs page here in the gammapy repo.

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 29, 2014

I've renamed the repo, you should now add the files in a new sub-folder here.

@ellisowen ellisowen closed this Jul 29, 2014

@ellisowen ellisowen reopened this Jul 29, 2014

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Jul 29, 2014

How is this? Please review

class FermiVelaRegion(object):
"""Fermi high-energy data for the Vela region.
TODO: document energy band, region, content of the files.

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Can you add a short description here (or link to the README file if it doesn't fit well here for some reason)?

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2014

The test fails for me: https://gist.github.com/cdeil/c19f2091ada9b6e42e9b
(you have to pass the --remote-data flag and test this locally, this is not tested on travis-ci)

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2014

The reason is that you are downloading an HTML page, not the raw FITS file.

The correct URL is e.g. https://github.com/gammapy/gammapy-extra/raw/master/datasets/vela_region/background_vela.fits

Maybe copy over the get_path utility function to gammapy/datasets/load.py from photutils?
https://github.com/astropy/photutils/blob/master/photutils/datasets/load.py#L42

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Jul 30, 2014

This should now address your comments

@@ -30,6 +30,7 @@
'diffuse_gamma_spectrum',
'electron_spectrum',
'FermiGalacticCenter',
'FermiVelaRegion',

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

FermiVelaRegion should be added to remote_datasets, not local_datasets

@staticmethod
def counts():
"""Counts image as `astropy.io.fits.ImageHDU`."""

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Add a Returns section and mention the returned object type astropy.io.fits.ImageHDU there.
Wouldn't it make more sense to return an HDUList?
This would give the user also easy access to the energy table extension.

Returns
-------
spectral_cube : `~gammapy.spectral_cube.GammaSpectralCube`

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

It's not true that you are returning a ~gammapy.spectral_cube.GammaSpectralCube ... at the moment it's an HDU object. I'm not sure which is better, ... you decide what is nicer for your use case, but make the docstring and code consistent.

Returns
-------
spectral_cube : `~gammapy.spectral_cube.GammaSpectralCube`

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Again ... the return type you list here is incorrect.

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2014

This works now.
I've left a few more inline comments ... this can be merged when those are addressed.

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Jul 30, 2014

OK, that I think addresses them

return result
@staticmethod
def counts():

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Rename counts -> counts_cube

return fits.HDUList([fits.open(filename)[0], fits.open(filename)[1])
@staticmethod
def background_cube():

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Rename background_cube -> background_image (I was surprised to see in the test that it's 2d.

* Max zenith angle cut: 105 deg
* 10 GeV < Energy < 500 GeV in 3 equal log10 bins
* Position: RA = 135.528583; DEC = -40.554694
* Image Radius: 1 degree

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Can you also mention the pixel size and that the image shape is (50, 50)?

* Event class and IRF: P7REP_CLEAN_V15
* Max zenith angle cut: 105 deg
* 10 GeV < Energy < 500 GeV in 3 equal log10 bins
* Position: RA = 135.528583; DEC = -40.554694

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Mention if the maps are in RADEC or GALACTIC system.

def test_background_cube(self):
background = FermiVelaRegion.background_cube()
assert background.data.shape == (50, 50)
assert_allclose(background.data.sum(), 351.66653)

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

So the diffuse background counts are higher than the actual counts (which include the Vela source)?
This doesn't look OK ... I guess you'll address this in a future PR, right?

This comment has been minimized.

@ellisowen

ellisowen Jul 30, 2014

Contributor

Yes, I agree. I'm not completely sure where my mistake is yet in producing the bg counts image & my desktop machine is currently broken so I can't investigate and produce a correct version just now, so will make another PR to correct this soon.

def test_exposure_cube(self):
exposure_cube = FermiVelaRegion.exposure_cube()
assert exposure_cube.data.shape == (4, 50, 50)
assert_quantity(exposure_cube.data.sum(), Quantity(1.4978096e+15, '1 / (cm2 MeV s sr)'))

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

This unit doesn't make sense for exposure.
I'd suggest you only assert on the the array sum, not the unit here.

Returns
-------
HDUList :

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Did you check that this looks OK by running python setup.py build_sphinx locally?
I think this might throw a Sphinx warning and make travis-ci fail.

This comment has been minimized.

@ellisowen

ellisowen Jul 30, 2014

Contributor

build_sphinx seems to work OK locally, I just tried it now. The output looks fine... I get one unrelated warning (below) but no failures:

/home/ellis/software/python/gammapy/docs/image/plotting.rst:21: WARNING: Exception occurred in plotting survey_example
from /home/ellis/software/python/gammapy/docs/image/survey_example.py:

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

I made this comment because the usual syntax is

name : type
    description

I still think listing the type as ~astropy.fits.io.HDUList would be an improvement, even if this works as is.

This comment has been minimized.

@ellisowen

ellisowen Jul 30, 2014

Contributor

Sure - will do this

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2014

You have to rebase this on master, because it conflicts with #152 that I just merged.

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Jul 30, 2014

How is this?

* Energy bins `~astropy.io.fits.BinTableHDU`.
"""
filename = FermiVelaRegion.filenames()['counts_cube']
return fits.HDUList([fits.open(filename)[0], fits.open(filename)[1]])

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Why not simply return this?

fits.open(filename)

This will be an HDUList ... I guess it contains an extra GTI extension, but having that can actually be useful, so that would be my suggestion.

* Max zenith angle cut: 105 deg
* 10 GeV < Energy < 500 GeV in 3 equal log10 bins
* CenterPosition: GLAT = 3.9298511274632784; GLON = 263.05836967702709
* Pixel size:

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

Remove this line ... pixel size is listed below.

* Event class and IRF: P7REP_CLEAN_V15
* Max zenith angle cut: 105 deg
* 10 GeV < Energy < 500 GeV in 3 equal log10 bins
* CenterPosition: GLAT = 3.9298511274632784; GLON = 263.05836967702709

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

List GLON first, then GLAT

@staticmethod
def background_image():
"""Predicted background counts spectral cube, based on the Fermi Diffuse model.

This comment has been minimized.

@cdeil

cdeil Jul 30, 2014

Member

"the Fermi diffuse model" is too vague.
You could simply add this link to the class docstring ... there is all the info, so you don't need to repeat everything here:
https://github.com/gammapy/gammapy-extra/tree/master/datasets/vela_region

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2014

Very close ... two more inline comments above.

@ellisowen

This comment has been minimized.

Contributor

ellisowen commented Jul 30, 2014

Made those changes...

@cdeil

This comment has been minimized.

Member

cdeil commented Jul 30, 2014

Thanks!

cdeil added a commit that referenced this pull request Jul 30, 2014

@cdeil cdeil merged commit fc829e7 into gammapy:master Jul 30, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

@cdeil cdeil changed the title from Fermi source data to Add Fermi Vela dataset Apr 8, 2015

@cdeil cdeil added the feature label Apr 8, 2015

@cdeil cdeil added this to the 0.1 milestone Apr 8, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment