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-LAT 3FHL catalogue #868

Merged
merged 5 commits into from Feb 5, 2017

Conversation

Projects
None yet
3 participants
@jjlk
Copy link
Contributor

jjlk commented Feb 4, 2017

Hi @cdeil and @adonath ,

I added two classes to handle the Fermi/LAT 3FHL catalogue. I followed again what did @adonath for the 3FGL catalogue and it seems to work (see attached plot for PKS 2155-304).

Here are some comments for this new catalogue:

  • integrated fluxes (like errors and energy fluxes) in energy bands are stored as vector in the Flux_Band column (Unc_Flux_Band, nuFnu)
  • energy bands are given in a table (EnergyBounds)
  • two different models are possible, power law and log parabola

In the SourceCatalog3FHL class I added columns to the point-like table to have integrated fluxes (and Co) as it is done in previous catalogs (e.g. Flux10_20GeV)

What do you think?

figure_1

@jjlk jjlk added the feature label Feb 4, 2017

@jjlk jjlk added this to the 0.6 milestone Feb 4, 2017

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 4, 2017

@jjlk - Thanks!

You probably saw this comment coming: "could you please add tests exercising the code for at least one example?" (or even for two if there's two spectral models in the catalog)

I'm also +1 to add your example script to the examples folder.
We will write docs or a notebook next week, but for now, having that code example lets one quickly try it out.

Otherwise, 👍 to merge without much review into master. We'll just try it out and if there's issues (e.g. flux points not filled correctly or something) we'll find it next week.

@cdeil cdeil self-assigned this Feb 4, 2017

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 4, 2017

@jjlk - If you have the time, I have one suggestion for the class docstrings.

I'd suggest that for 3FHL and all Fermi catalogs, we link the catalog and object docstring like in this example:

That makes it convenient to browse the API docs.

If you don't want to do this here, I can do it in a follow-up PR.

def spectrum(self):
"""Spectrum model fit result (`~gammapy.spectrum.SpectrumFitResult`)
TODO: remove!???

This comment has been minimized.

@cdeil

cdeil Feb 4, 2017

Member

Suggest to remove this TODO comment here and from the other Fermi-LAT catalogs, and instead make a Github issue with Axel, Johannes and me in CC asking the question if the way the catalogs expose the "spectrum fit result" and "energy range" info is good or should be changed / improved.

We're thinking about making the API better, but this TODO comment is not useful for users.

For now you can milestone this one 0.6, and if we don't find something better quickly, we'll move to 0.7.

for column in ['eflux', 'eflux_errp', 'eflux_errn']:
table[column][is_ul] = np.nan

# TODO: check if nuFnu is maybe integral flux

This comment has been minimized.

@cdeil

cdeil Feb 4, 2017

Member

Can you resolve this TODO comment?

In the FITS file I see

$ ftlist gll_psch_v11.fit.gz[1] C
HDU 2  

  Col  Name             Format[Units](Range)      Comment
    1 Source_Name        18A                  
    2 RAJ2000            E [deg] (0.000000:360.000) 
    3 DEJ2000            E [deg] (-90.0000:90.0000) 
    4 GLON               E [deg] (0.000000:360.000) 
    5 GLAT               E [deg] (-90.0000:90.0000) 
    6 Conf_95_SemiMajor  E [deg] (0.000000:)  
    7 Conf_95_SemiMinor  E [deg] (0.000000:)  
    8 Conf_95_PosAng     E [deg]              
    9 ROI_num            I                    
   10 Signif_Avg         E (0.000000:)        
   11 Pivot_Energy       E [GeV] (0.000000:)  
   12 Flux_Density       E [photon/cm**2/GeV/s] (0.000000:) 
   13 Unc_Flux_Density   E [photon/cm**2/GeV/s] (0.000000:) 
   14 Flux               E [photon/cm**2/s]   
   15 Unc_Flux           E [photon/cm**2/s]   
   16 Energy_Flux        E [erg/cm**2/s]      
   17 Unc_Energy_Flux    E [erg/cm**2/s]      
   18 Signif_Curve       E                    
   19 SpectrumType       11A                  
   20 Spectral_Index     E (0.000000:10.0000) 
   21 Unc_Spectral_Index E (0.000000:)        
   22 beta               E (-1.00000:1.00000) 
   23 Unc_beta           E (0.000000:)        
   24 PowerLaw_Index     E (0.000000:10.0000) 
   25 Unc_PowerLaw_Index E (0.000000:)        
   26 Flux_Band          5E [photon/cm**2/s]  
   27 Unc_Flux_Band      10E [photon/cm**2/s] 
   28 nuFnu              5E [erg/cm**2/s]     
   29 Sqrt_TS_Band       5E                   
   30 Npred              E (0.000000:)        
   31 HEP_Energy         E [GeV]              
   32 HEP_Prob           E (0.000000:1.00000) 
   33 Variability_BayesBlocks I                    
   34 Extended_Source_Name 18A                  
   35 ASSOC_GAM          18A                  
   36 TEVCAT_FLAG        A                    
   37 ASSOC_TEV          21A                  
   38 CLASS              7A                   
   39 ASSOC1             26A                  
   40 ASSOC2             26A                  
   41 ASSOC_PROB_BAY     E (0.000000:1.00000) 
   42 ASSOC_PROB_LR      E (0.000000:1.00000) 
   43 Redshift           E                    
   44 NuPeak_obs         E [Hz]               

So it's clear that nuFnu is E^2 * dN/dE and not an integral flux, no?

If it's unclear to you, or if the question is really whether we should access that field or other fields, let's figure that out before merging this PR.

@cdeil
Copy link
Member

cdeil left a comment

I've left two inline comments about the "TODO" I saw.

@jjlk

This comment has been minimized.

Copy link
Contributor

jjlk commented Feb 4, 2017

Ola @cdeil, Sorry for the test, I added one. Plus an example script (which is not optimal). I removed some ToDos (in spectrum()).

For the class docstrings, why not. I'll be available to do some doc fixes and improve tutorials starting from next Tuesday. I'm pretty busy right now =)

For the nuFnu column, it looks like a differential flux. So I guess there is no problem, right? Just remove the ToDo?

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 4, 2017

For the nuFnu column, it looks like a differential flux. So I guess there is no problem, right? Just remove the ToDo?

The description in the paper draft (Table 3) is vague: "Spectral energy distribution over each spectral band"
I'm not sure.

My suggestion would be to remove the TODO comments, and to create a new issue assigned to Axel, with me in CC, to review how exactly flux points are filled for all Fermi-LAT catalogs.
Then this PR can go in now, and we'll have the reminder to check this next week.

@cdeil
Copy link
Member

cdeil left a comment

Travis-ci fails are unrelated (looks like master is broken since yesterday).

@jjlk - Merge? Or is there something you still want to do here?

""" Example to show how to plot spectrum of Fermi/LAT sources
"""

"""Example how to plot Fermi-LAT catalog spectra.

This comment has been minimized.

@cdeil

cdeil Feb 4, 2017

Member

This example file has two docstrings as the top.
I like your enthusiasm for documentation!

@jjlk

This comment has been minimized.

Copy link
Contributor

jjlk commented Feb 5, 2017

Hi @cdeil ,
I Removed the ToDos concerning the flux points and I fixed the overloaded docstring =)

I'll open an issue for the computation of flux points. OKé to merge?

@cdeil cdeil changed the title 3FHL catalogue Add Fermi-LAT 3FHL catalogue Feb 5, 2017

@cdeil

cdeil approved these changes Feb 5, 2017

@cdeil cdeil merged commit fe4d862 into gammapy:master Feb 5, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 5, 2017

@jjlk - Thanks!


Unrelated to this PR: @bsipocz - the travis-ci build for egg_info failed, but it says exit code 0 at the end and I don't understand why it's marked as failed. Could you have a look, please? https://travis-ci.org/gammapy/gammapy/jobs/198553244

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Feb 5, 2017

It's a known upstream travis issue, I'm afraid we can't do anything but wait for them to fix it. More details: travis-ci/travis-ci#7264 (comment)

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented Feb 5, 2017

Actually it seems that this should have been fixed already, yet the new merge build is still failing :(

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 5, 2017

@bsipocz - Thanks for the info!

No problem from our side if they fix it in the coming days.

@cdeil

This comment has been minimized.

Copy link
Member

cdeil commented Feb 5, 2017

I've done some code clean-up in 631131b (mainly sort classes and tests in a good order, the file is getting long), and cross-linked the catalog and source class docstrings in 322c13d.

@jjlk jjlk deleted the jjlk:fermi branch Feb 8, 2017

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