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 1FHL catalogue #855

Merged
merged 2 commits into from Jan 30, 2017

Conversation

Projects
None yet
3 participants
@jjlk
Contributor

jjlk commented Jan 29, 2017

Hi @adonath and @cdeil ,
I added the two classes needed to handle the 1FHL catalogue. I followed closely what @adonath did for the 2FHL catalogue. Is it ok to merge?

Thanks!

PS: I noticed that the butterflies for the 2FHL and the 1FHL sources where lacking some curvatures (see attached plot for PKS 2155-304). In addition the upper limits do not appear (here for the 2FHL). Is it something expected?

pks2155

@cdeil cdeil self-assigned this Jan 29, 2017

@cdeil cdeil added the feature label Jan 29, 2017

@cdeil cdeil added this to the 0.6 milestone Jan 29, 2017

@cdeil cdeil changed the title from 1FHL catalogue to Add 1FHL catalogue Jan 29, 2017

@@ -340,12 +345,12 @@ def spectrum(self):
def _get_flux_values(self, prefix='Flux', unit='cm-2 s-1'):
if prefix not in ['Flux', 'Unc_Flux', 'nuFnu']:
raise ValueError("Must be one of the following: 'Flux', 'Unc_Flux', 'nuFnu'")
raise ValueError(
"Must be one of the following: 'Flux', 'Unc_Flux', 'nuFnu'")

This comment has been minimized.

@cdeil

cdeil Jan 29, 2017

Member

You've added a lot of line breaks, presumably where the length was a little over the 80 char limit.
Did you do this manually or did you editor do this automatically?
(I prefer to be not too strict with the line length, in cases where it's just a little over and less readable when you break.)

@cdeil

cdeil Jan 29, 2017

Member

You've added a lot of line breaks, presumably where the length was a little over the 80 char limit.
Did you do this manually or did you editor do this automatically?
(I prefer to be not too strict with the line length, in cases where it's just a little over and less readable when you break.)

This comment has been minimized.

@jjlk

jjlk Jan 29, 2017

Contributor

Sorry about that. Yes my editor did that... What is the standard?

@jjlk

jjlk Jan 29, 2017

Contributor

Sorry about that. Yes my editor did that... What is the standard?

This comment has been minimized.

@cdeil

cdeil Jan 29, 2017

Member

PyCharm by default fixes some stuff (like whitespace, ...) but leaves line breaks alone.
If you can configure your editor that way, and make pull requests that don't have stuff in the diff with unrelated changes (and in this case even changed i don't like), that would be good.

But it's not a big deal, we can still merge if you don't want to revert here now.

@cdeil

cdeil Jan 29, 2017

Member

PyCharm by default fixes some stuff (like whitespace, ...) but leaves line breaks alone.
If you can configure your editor that way, and make pull requests that don't have stuff in the diff with unrelated changes (and in this case even changed i don't like), that would be good.

But it's not a big deal, we can still merge if you don't want to revert here now.

@cdeil

This looks good to me.

The pull request could be extended with fixes for the missing flux point ULs in the plot or butterfly issue you mention, or by adding your nice example to http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html or as a separate file in examples for now.

But it's up to you, that can also be done in future PRs.

def __str__(self):
"""Print summary info."""
# TODO: can we share code with 3FGL summary funtion?

This comment has been minimized.

@cdeil

cdeil Jan 29, 2017

Member

Did you add this TODO or is this copy & pasted?

Yes, there are ways to avoid duplicating code:

  • Make a utility function and call it from several places
  • Make a base class that has this utility method (SourceCatalogObjectFermi in this case), and call it from subclasses

Copy & paste is bad, but more functions and classes is extra complexity.
Personally my threshold to factor out common code is pretty low and I would do it in this case, but it's a matter of style.

@cdeil

cdeil Jan 29, 2017

Member

Did you add this TODO or is this copy & pasted?

Yes, there are ways to avoid duplicating code:

  • Make a utility function and call it from several places
  • Make a base class that has this utility method (SourceCatalogObjectFermi in this case), and call it from subclasses

Copy & paste is bad, but more functions and classes is extra complexity.
Personally my threshold to factor out common code is pretty low and I would do it in this case, but it's a matter of style.

This comment has been minimized.

@jjlk

jjlk Jan 29, 2017

Contributor

It's copy pasted. I guess the answer depends if all Fermi/LAT catalogues have the same definition for RA, DEC and so on (I think yes for now but we will see with the 3FHL catalogue, right?).

@jjlk

jjlk Jan 29, 2017

Contributor

It's copy pasted. I guess the answer depends if all Fermi/LAT catalogues have the same definition for RA, DEC and so on (I think yes for now but we will see with the 3FHL catalogue, right?).

This comment has been minimized.

@cdeil

cdeil Jan 29, 2017

Member

Yeah, I don't know.

My guess would be that code re-use will be limited, and one could just write this pretty-print output top to bottom like we did in the HGPS source class.
If no-one has time for now, an alternative would be to put this at the end:

from pprint import pprint
pprint(self.data)

so that at least all info is printed, one item per line.
Or put it into an Astropy table with two columsn "Parameter" and "Value" and call pprint on that.

In practice this is really nice to have as you look around the catalogs, and should be added to the docs.

But also, mostly unrelated to your PR here, so OK to just keep as-is, unless you want to add this convenience.

@cdeil

cdeil Jan 29, 2017

Member

Yeah, I don't know.

My guess would be that code re-use will be limited, and one could just write this pretty-print output top to bottom like we did in the HGPS source class.
If no-one has time for now, an alternative would be to put this at the end:

from pprint import pprint
pprint(self.data)

so that at least all info is printed, one item per line.
Or put it into an Astropy table with two columsn "Parameter" and "Value" and call pprint on that.

In practice this is really nice to have as you look around the catalogs, and should be added to the docs.

But also, mostly unrelated to your PR here, so OK to just keep as-is, unless you want to add this convenience.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 29, 2017

Member

Concerning why the upper limits of the spectral points don't show up:

In principle the FluxPoints class can handle that, and I think it's working for the GammaCat and HGPS catalog classes:
http://docs.gammapy.org/en/latest/_modules/gammapy/spectrum/flux_point.html#FluxPoints.plot

But looking at the docs and tests, I don't see a working example of that for the Fermi spectral points:
http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html

I just did this demo today for @vorugantia to explain a few things, how to get started fitting spetral points with Gammapy:
http://nbviewer.jupyter.org/github/vorugantia/scienceresearch11/blob/master/notebooks/spectrum_chi2_demo_from_christoph.ipynb

The first issue we ran into is that the Fermi catalogs contain integral flux points, but for plotting we wanted differential flux points and differential flux point errors.

What you do here (presumably copied from 2FHL class) to compute dnde in the flux_points method, but to not fill dnde error is confusing IMO.

IMO it would be better to not compute dnde in the flux_points method on the Catalog object, but to add a to_differential_flux_points method.
@jjlk @adonath - Thoughts?

Apparently @adonath already started in that direction and wrote a test for it here that is xfailed at the moment:
https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/tests/test_flux_point.py#L153
@jjlk or @adonath - If you have time to make follow-up PRs next week to finish of the Fermi catalog flux point stuff, and improve the docs to also show examples with ULs, that would be great!

Member

cdeil commented Jan 29, 2017

Concerning why the upper limits of the spectral points don't show up:

In principle the FluxPoints class can handle that, and I think it's working for the GammaCat and HGPS catalog classes:
http://docs.gammapy.org/en/latest/_modules/gammapy/spectrum/flux_point.html#FluxPoints.plot

But looking at the docs and tests, I don't see a working example of that for the Fermi spectral points:
http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html

I just did this demo today for @vorugantia to explain a few things, how to get started fitting spetral points with Gammapy:
http://nbviewer.jupyter.org/github/vorugantia/scienceresearch11/blob/master/notebooks/spectrum_chi2_demo_from_christoph.ipynb

The first issue we ran into is that the Fermi catalogs contain integral flux points, but for plotting we wanted differential flux points and differential flux point errors.

What you do here (presumably copied from 2FHL class) to compute dnde in the flux_points method, but to not fill dnde error is confusing IMO.

IMO it would be better to not compute dnde in the flux_points method on the Catalog object, but to add a to_differential_flux_points method.
@jjlk @adonath - Thoughts?

Apparently @adonath already started in that direction and wrote a test for it here that is xfailed at the moment:
https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/tests/test_flux_point.py#L153
@jjlk or @adonath - If you have time to make follow-up PRs next week to finish of the Fermi catalog flux point stuff, and improve the docs to also show examples with ULs, that would be great!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 29, 2017

Member

PS: @adonath changed the gammapy.spectrum.FluxPoints class a while ago to be in line with the spec, which means upper limits should be stored like this using NaN values:
https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/master/source/results/flux_points/diff_flux_points.ecsv

This is encoded here to decide which points are upper limits:
https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/flux_point.py#L255

I think the flux_points methods in the Fermi catalog classes haven't been updated yet to follow that format, and probably that's why upper limits aren't plotted correctly.

Could you please try to find a source that has an UL for each Fermi catalog, and make sure that it's filled and plotted correctly?

About the incorrect butterfly -- I suggest you open a new issue and label it as "bug" and put it under the 0.6 milestone, assigning it to Axel, unless you want to take care of it.

Member

cdeil commented Jan 29, 2017

PS: @adonath changed the gammapy.spectrum.FluxPoints class a while ago to be in line with the spec, which means upper limits should be stored like this using NaN values:
https://github.com/open-gamma-ray-astro/gamma-astro-data-formats/blob/master/source/results/flux_points/diff_flux_points.ecsv

This is encoded here to decide which points are upper limits:
https://github.com/gammapy/gammapy/blob/master/gammapy/spectrum/flux_point.py#L255

I think the flux_points methods in the Fermi catalog classes haven't been updated yet to follow that format, and probably that's why upper limits aren't plotted correctly.

Could you please try to find a source that has an UL for each Fermi catalog, and make sure that it's filled and plotted correctly?

About the incorrect butterfly -- I suggest you open a new issue and label it as "bug" and put it under the 0.6 milestone, assigning it to Axel, unless you want to take care of it.

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Jan 29, 2017

Contributor

Oké let's do that, I'll open an issue.

Oké to merge?

Contributor

jjlk commented Jan 29, 2017

Oké let's do that, I'll open an issue.

Oké to merge?

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 29, 2017

Member

Oké to merge?

Maybe remove the addition of the dnde column in the Fermi catalogs, and add an is_ul column so that ULs are filled correctly? If you don't have time, OK to merge, but then please also file a reminder issue.

Member

cdeil commented Jan 29, 2017

Oké to merge?

Maybe remove the addition of the dnde column in the Fermi catalogs, and add an is_ul column so that ULs are filled correctly? If you don't have time, OK to merge, but then please also file a reminder issue.

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Jan 29, 2017

Contributor

I opened an issue (#856) and I'll try to fix this next week if @adonath has no time.

Contributor

jjlk commented Jan 29, 2017

I opened an issue (#856) and I'll try to fix this next week if @adonath has no time.

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jan 30, 2017

Member

@cdeil @jjlk There's a helper function compute_flux_points_dnde, which computes differential flux point quantities from integral ones. As long as an integral error is given it takes care of the error propagation to compute dnde_err or dnde_errp and dnde_errn as well.

Member

adonath commented Jan 30, 2017

@cdeil @jjlk There's a helper function compute_flux_points_dnde, which computes differential flux point quantities from integral ones. As long as an integral error is given it takes care of the error propagation to compute dnde_err or dnde_errp and dnde_errn as well.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 30, 2017

Member

I'm merging this now.

@adonath @jjlk - If you could fix / improve the points discussed here this week, that would be great and IMO a high-priority item. Even adding a FluxPointFitter that can fit differential and integral flux points, possibly with an API as outlined in the notebook that I linked to in @vorugantia's repo, would be important to add soon, because we and several other people need this.

Member

cdeil commented Jan 30, 2017

I'm merging this now.

@adonath @jjlk - If you could fix / improve the points discussed here this week, that would be great and IMO a high-priority item. Even adding a FluxPointFitter that can fit differential and integral flux points, possibly with an API as outlined in the notebook that I linked to in @vorugantia's repo, would be important to add soon, because we and several other people need this.

@cdeil cdeil merged commit e3af112 into gammapy:master Jan 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jjlk jjlk deleted the jjlk:fermi branch Jan 31, 2017

@cdeil cdeil changed the title from Add 1FHL catalogue to Add Fermi-LAT 1FHL catalogue Feb 5, 2017

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