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

Add printout for 3FHL and gamma-cat sources #1078

Merged
merged 7 commits into from Jul 5, 2017
Merged

Conversation

@vorugantia
Copy link
Contributor

@vorugantia vorugantia commented Jun 30, 2017

@adonath @cdeil - This PR mainly focuses on the __str__() method for SourceCatalogObject3FHL, but here is everything it accomplishes:

  • Create full print-out for SourceCatalogObject3FHL

  • Add new property, _flux_points_table_formatted, to 3FHL and 3FGL source objects - essentially just a cleaner version of flux_points.table. It allows people to easily dump a clean + formatted astropy table to JSON, etc.

  • Minor fixes to print-outs for 3FGL and HGPS sources (e.g. add _flux_points_table_formatted to print string)

  • Add sqrt_TS column to flux_points.table for 3FHL and 3FGL source objects

I haven't really done anything with the tests for SourceCatalogObject3FHL and SourceCatalogObject3FGL yet - let me know what can be done for them.

@vorugantia
Copy link
Contributor Author

@vorugantia vorugantia commented Jun 30, 2017

Here's the current output for print(<3fhl source>):

*** Basic info ***

Source                         : 3FHL J0534.5+2201
Catalog row index (zero-based) : 352
Extended name                  :
Associations     : PSR J0534+2200, Crab Pulsar, 3FGL J0534.5+2201i
ASSOC_PROB_BAY   : 1.000
ASSOC_PROB_LR    : 1.000
Class                          : PSR
TeVCat flag                    : Small TeV source

*** Position info ***

RA                   : 83.635 deg
DEC                  : 22.019 deg
GLON                 : 184.554 deg
GLAT                 : -5.780 deg

Semimajor (95%)      : 0.0080 deg
Semiminor (95%)      : 0.0080 deg
Position angle (95%) : nan deg
ROI number           : 430

*** Model info ***

Detection significance (10 GeV - 2 TeV)       : 168.641
Npred                                         : 2602.9

Spectrum type                    : PowerLaw
Significance curvature           : 1.4
Spectral index                   : 2.285 +- 0.038
Pivot energy                     : 23 GeV
Power Law index                  : 2.220 +- 0.025
Flux Density at pivot energy     : 1.71e-10 +- 3.39e-12 1 / (cm2 GeV s)
Integral flux (10 GeV - 1 TeV)   : 8.66e-09 +- 1.71e-10 1 / (cm2 s)
Energy flux (10 GeV - TeV)       : 4.91e-10 +- 1.66e-11 erg / (cm2 s)

*** Spectral points ***

e_ref  e_min e_max      flux     flux_errn   flux_errp      eflux       eflux_errn    eflux_errp  is_ul   flux_ul      eflux_ul   sqrt_TS       dnde
 GeV    GeV   GeV   1 / (cm2 s) 1 / (cm2 s) 1 / (cm2 s) erg / (cm2 s) erg / (cm2 s) erg / (cm2 s)       1 / (cm2 s) erg / (cm2 s)         1 / (cm2 s TeV)
------ ----- ------ ----------- ----------- ----------- ------------- ------------- ------------- ----- ----------- ------------- ------- ---------------
  14.1  10.0   20.0    5.17e-09    1.33e-10    1.33e-10      1.64e-10      4.24e-12      4.24e-12 False         nan           nan   125.2        5.12e-07
  31.6  20.0   50.0    2.25e-09    8.67e-11    8.67e-11      1.18e-10      4.56e-12      4.56e-12 False         nan           nan    88.7        7.37e-08
  86.6  50.0  150.0    9.24e-10     5.5e-11     5.5e-11      1.09e-10      6.46e-12      6.46e-12 False         nan           nan    59.1        9.04e-09
 273.9 150.0  500.0    2.76e-10    2.92e-11    3.14e-11      9.23e-11      9.76e-12      1.05e-11 False         nan           nan    33.1        7.68e-10
1000.0 500.0 2000.0    6.68e-11    1.46e-11    1.69e-11       6.9e-11      1.51e-11      1.75e-11 False         nan           nan    15.6        4.31e-11

*** Other info ***

HEP Energy       : 1463.300 GeV
HEP Probability  : 1.000
Variability - Bayesian Blocks  : Not variable
Redshift         : nan
NuPeak_obs       : nan Hz
@vorugantia vorugantia changed the title __str__() methods for 3FHL, 3FGL, HGPS Printout for 3FHL, 3FGL, HGPS catalog objects Jun 30, 2017
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 30, 2017

Printout looks good!
One change I would suggest is to put cm-2 GeV-1 s-1 instead of 1 / (cm2 GeV s) (even if not a very common way to write it), because I find the latter hard to read with the float number followed b the 1 /. Example:

Flux Density at pivot energy     : 1.71e-10 +- 3.39e-12 1 / (cm2 GeV s)
@cdeil cdeil self-assigned this Jun 30, 2017
@cdeil cdeil added the feature label Jun 30, 2017
@cdeil cdeil added this to the 0.7 milestone Jun 30, 2017
@cdeil cdeil changed the title Printout for 3FHL, 3FGL, HGPS catalog objects Add printout for 3FHL and gamma-cat sources Jun 30, 2017
@vorugantia vorugantia force-pushed the vorugantia:3fhl_str branch from ea644cd to 5bf7b23 Jun 30, 2017
@vorugantia
Copy link
Contributor Author

@vorugantia vorugantia commented Jul 3, 2017

@cdeil - The changes you mentioned above have been implemented.

I also started the printout for GammaCat source objects, here is what the output looks like right now:

*** Basic info ***

Catalog row index (zero-based) : 36
Common name     : Vela X
Other names     : HESS J0835-455
Location        : gal
Class           : pwn

TeVCat ID       : 86
TeVCat 2 ID     : yVoFOS
TeVCat name     : TeV J0835-456

TGeVCat ID      : 37
TGeVCat name    : TeV J0835-4536

*** Position info ***

SIMBAD:
RA                   : 128.287 deg
DEC                  : -45.190 deg
GLON                 : 263.332 deg
GLAT                 : -3.106 deg

Measurement:
RA                   : 128.750 deg
DEC                  : -45.600 deg
GLON                 : 263.856 deg
GLAT                 : -3.089 deg
Position error       : nan deg

*** Morphology info ***

Morphology model type     : gauss
Sigma                     : 0.480 deg
Sigma error               : 0.030 deg
Sigma2                    : 0.360 deg
Sigma2 error              : 0.030 deg
Position angle            : 41.000 deg
Position angle error      : 7.000 deg
Position angle frame      : radec


Derived fluxes:
Spectral model norm (1 TeV)    : 1.36e-11 +- 7.53e-13 cm-2 s-1 TeV-1 (statistical)
Integrated flux (<1 TeV)       : 2.1e-11 +- 1.97e-12 cm-2 s-1 (statistical)
Integrated flux (<1 TeV)       : 1.01e+02 +- 9.51 (crab units)
Integrated flux (1-10 TeV)     : 9.27e-11 +- 9.59e-12 erg cm-2 s-1 (statistical)

*** Spectral points ***
SED reference id     : 2012A&A...548A..38A
# spectral points    : 24
# upper limits       : 0

e_ref       dnde         dnde_errn       dnde_errp
 TeV  1 / (cm2 s TeV) 1 / (cm2 s TeV) 1 / (cm2 s TeV)
----- --------------- --------------- ---------------
  0.7        1.06e-11        3.28e-12        3.28e-12
  0.9         1.3e-11        2.13e-12        2.13e-12
  1.1        9.21e-12         1.4e-12         1.4e-12
  1.3        8.52e-12        9.58e-13        9.61e-13
  1.5        5.38e-12        7.07e-13        7.09e-13
  1.9        4.46e-12        5.05e-13        5.07e-13
  2.3        3.75e-12         3.3e-13        3.34e-13
  2.8        2.42e-12        2.68e-13         2.7e-13
  3.4         1.6e-12         1.8e-13        1.83e-13
  4.1        1.45e-12        1.26e-13        1.29e-13
  5.0        9.24e-13        9.49e-14         9.7e-14
  6.0        7.35e-13        6.47e-14        6.71e-14
  7.3        3.86e-13        4.54e-14         4.7e-14
  8.8        3.58e-13        3.57e-14        3.75e-14
 10.6         1.7e-13        2.49e-14        2.59e-14
 12.9        1.55e-13        2.06e-14        2.16e-14
 15.6        6.69e-14        1.13e-14        1.23e-14
 18.9        2.11e-14        1.39e-14        1.32e-14
 22.6        3.28e-14        6.83e-15        7.51e-15
 26.9        3.03e-14        5.91e-15        6.66e-15
 31.6        1.86e-14        4.38e-15        5.12e-15
 37.0        5.65e-15        2.17e-15        2.92e-15
 43.1        3.48e-15        1.64e-15        2.41e-15
 52.4           1e-15        8.33e-16        1.62e-15

*** Reference info ***

Discoverer           : hess
Discovery date       : 2006-03
Seen by              : hess
Reference            : 2012A&A...548A..38A

Let me know how it looks. I'll implement tests for the GammaCat print method tomorrow.

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 4, 2017

The gamma-cat output looks good to me. I only have one suggestion: move the "reference info" at the end after the "basic info" at the top, or even merge the two sections if you prefer.

Then as you say: add or improve a test and it's ready to be merged.

This is what this source looks like in TeVCat:

From a quick look, it seems that source distance is something we're missing in the catalog, maybe there's other info? My suggestion would be that we proceed like this:

  • Dump all info we have now to JSON (a pull request to Gammapy that we should do together)
  • Add HTML views for 3FHL, 3FGL and gamma-at to gamma-sky.net (something you can largely do by yourself)
  • then go back to gamma-cat and add the few missing infos, like source distance or infos on the papers (extract from ADS with a script)
Copy link
Member

@cdeil cdeil left a comment

I've left a few inline comments with small suggestions. Let me know if you have any questions.

@@ -319,6 +306,24 @@ def spectral_model(self):
return model

@property
def _flux_points_table_formatted(self):
"""Returns formatted version of self.flux_points.table"""
table = self.flux_points.table.copy()

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

I'd suggest to shorten this method a bit:

  • remove all empty lines (empty lines are only useful if they separate steps, just an empty line between every line of code is uncommon)
  • Break the line with the long list earlier
  • Use a list comprehension for
[table[_].format = '.3' for _ in flux_cols]

because it's just as readable but one line shorter.

@@ -352,13 +357,21 @@ def flux_points(self):
eflux_ul = compute_flux_points_ul(table['eflux'], table['eflux_errp'])
table['eflux_ul'][is_ul] = eflux_ul[is_ul]

# Square root of test statistic
ts = self._get_ts_values()

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

I think you can just do this and remove the method here:

table['sqrt_ts'] = [self.data['Sqrt_TS' + _] for _ in self._ebounds_suffix]

TS doesn't have a unit, so I don't think you need to call Quantity.

ss += '{:<16s} : {:.3f}\n'.format('HEP Probability', d['HEP_Prob'])

bayes = d['Variability_BayesBlocks']
str = bayes

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

This code can be improved a bit:

  • First of all str is a Python built-in keyword. Overwriting Python built-ins with variable names is considered bad practice, so you should choose some other variable name here like msg or message or anything else you like.
  • Secondly, you're creating two local variables "str" and "bayes", but you really only need one.
@@ -632,9 +817,15 @@ def flux_points(self):
eflux_ul = compute_flux_points_ul(table['eflux'], table['eflux_errp'])
table['eflux_ul'][is_ul] = eflux_ul[is_ul]

# Square root of test statistic
ts = self.data['Sqrt_TS_Band']
table['sqrt_TS'] = ts

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

No need for a local variable, you can do it in one line:

table['sqrt_ts'] = self.data['Sqrt_TS_Band']
ss += '\n{:<15s} : {} {}\n'.format('Spectrum type', spec, str)

# Spectral model parameters
if(spec == 'pl'):

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

You don't need parens around expressions in the if statenemts. Remove.
Also: use the if - elif - elif - else pattern ending with:

else:
    raise ValueError('Spectral model printout not implemented: {}'.format(spec))

Long-term, it might be better to instantiate spectral model objects and just print out those.
But for now, what you have here is find I think (I don't remember what we do for the other catalogs to print out the spectral models at the moment).

ss += '{:<20s} : {}\n'.format('# spectral points', d['sed_n_points'])
ss += '{:<20s} : {}\n\n'.format('# upper limits', d['sed_n_ul'])

t = self._flux_points_table_formatted

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

I think there are sources without spectral points, where this will raise an error?
If so, I think the whole section should be in an if or try statement, and if there are no spectral points, just print "no spectral points available for this source" instead of erroring out.

To test all sources you should simply be able to do this:

for source in catalog:
    print(source.name)
    print(source)
def _flux_points_table_formatted(self):
"""Returns formatted version of self.flux_points.table"""
table = self.flux_points.table.copy()

This comment has been minimized.

@cdeil

cdeil Jul 4, 2017
Member

Again, please shorten this a bit (remove empty lines, use list comprehension for setting formats).

@vorugantia
Copy link
Contributor Author

@vorugantia vorugantia commented Jul 4, 2017

Okay, I've made the changes you mentioned and cleaned up the code a little bit. All tests are now working in test_fermi.py and test_gammacat.py.

@cdeil cdeil force-pushed the vorugantia:3fhl_str branch from 40ef534 to d14aadb Jul 4, 2017
@cdeil
Copy link
Member

@cdeil cdeil commented Jul 4, 2017

We reviewed this together:

One remaining addition: for 3FHL, add printout for extended sources. Something like this:

if not source.is_pointlike:
     print out stuff from source.data_extended

and a test for at least one of the extended sources if you don't have it already.

@cdeil cdeil dismissed their stale review Jul 4, 2017

all done

@vorugantia
Copy link
Contributor Author

@vorugantia vorugantia commented Jul 4, 2017

I've added more print lines for extended 3FHL sources. Everything should be good now!

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 4, 2017

@vorugantia - I was expecting a change to the tests as well, adding at least one assert line for the printout from this new part for extended sources.

@vorugantia vorugantia force-pushed the vorugantia:3fhl_str branch from f6dddc6 to 76ad473 Jul 5, 2017
@vorugantia
Copy link
Contributor Author

@vorugantia vorugantia commented Jul 5, 2017

Okay, new test has been added.

@cdeil
Copy link
Member

@cdeil cdeil commented Jul 5, 2017

Thanks!

Merging this now.

@cdeil cdeil merged commit cb6ff89 into gammapy:master Jul 5, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants