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

Fix Fermi catalog flux point upper limits #858

Merged
merged 4 commits into from Feb 1, 2017

Conversation

Projects
None yet
3 participants
@adonath
Member

adonath commented Jan 30, 2017

This PR addresses the issues described in #856:

  1. Fix an issue with the covariance matrix in SourceCatalogObject2FHL and SourceCatalogObject1FHL.
  2. Add upper limit information in to the flux points

For PKS 2155-304 the plot looks now as following:
fermi_pks_2155_304

@jjlk Does it look allright to you?

@adonath adonath self-assigned this Jan 30, 2017

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Jan 30, 2017

Contributor

Hi @adonath , it looks awsome =)

Contributor

jjlk commented Jan 30, 2017

Hi @adonath , it looks awsome =)

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

@cdeil cdeil changed the title from Fix for #856 to Fix Fermi catalog flux point upper limits Jan 31, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 31, 2017

Member

@adonath - Thank you!

Travis-ci passed, I'm merging this now.

In the test removed here you had int_points.to_differential_flux_points.
Like I said, I think it would be nice to expose that as a method (and probably the other way around should be added as well).
Or do you prefer to only have it as a standalone function?

In any case, the int -> diff flux conversion should be pointed out to users, as it's something to be aware of when plotting and analysing flux points.
So I'm +1 to change the docs examples to have int -> diff as an explicit step, e.g. here:
http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html

Member

cdeil commented Jan 31, 2017

@adonath - Thank you!

Travis-ci passed, I'm merging this now.

In the test removed here you had int_points.to_differential_flux_points.
Like I said, I think it would be nice to expose that as a method (and probably the other way around should be added as well).
Or do you prefer to only have it as a standalone function?

In any case, the int -> diff flux conversion should be pointed out to users, as it's something to be aware of when plotting and analysing flux points.
So I'm +1 to change the docs examples to have int -> diff as an explicit step, e.g. here:
http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html

@cdeil cdeil added the bug label Jan 31, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 31, 2017

Member

The docs example with an upper limit is nice.

Can we change the source at to one that has upper limits:
http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html
or, for now, just add the script you developed here to examples?

Member

cdeil commented Jan 31, 2017

The docs example with an upper limit is nice.

Can we change the source at to one that has upper limits:
http://docs.gammapy.org/en/latest/spectrum/plotting_fermi_spectra.html
or, for now, just add the script you developed here to examples?

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Jan 31, 2017

Member

@cdeil I will add a method FluxPoints.to_flux_points_dnde() in a sperate PR and change (or add) the docs example accordingly.

Member

adonath commented Jan 31, 2017

@cdeil I will add a method FluxPoints.to_flux_points_dnde() in a sperate PR and change (or add) the docs example accordingly.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Jan 31, 2017

Member

@adonath - Thanks!

I've been thinking about whether FluxPoints.to_dnde() should modify the object in-place or return a new object, with a copy of the table, i.e. no link back. And whether both directions to_dnde and to_flux should be added.

I think I'd be +1 to "yes" on both questions, but I don't see strong pros and cons yet. So do as you think best, just wanted to mention it, or get an opinion from @jjlk or @joleroi on what we should do, before you spend the time to implement it and modify docs / tests.

Member

cdeil commented Jan 31, 2017

@adonath - Thanks!

I've been thinking about whether FluxPoints.to_dnde() should modify the object in-place or return a new object, with a copy of the table, i.e. no link back. And whether both directions to_dnde and to_flux should be added.

I think I'd be +1 to "yes" on both questions, but I don't see strong pros and cons yet. So do as you think best, just wanted to mention it, or get an opinion from @jjlk or @joleroi on what we should do, before you spend the time to implement it and modify docs / tests.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Feb 1, 2017

Member

@adonath - Did we forget to merge this? Or are you still working in this branch?

Member

cdeil commented Feb 1, 2017

@adonath - Did we forget to merge this? Or are you still working in this branch?

@cdeil cdeil merged commit ffbf706 into gammapy:master Feb 1, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment