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 two bugs in LightCurveEstimator, and improve speed considerably #1179

Merged
merged 2 commits into from Nov 3, 2017

Conversation

Projects
None yet
3 participants
@lmohrmann
Contributor

lmohrmann commented Oct 20, 2017

This PR fixes two bugs:

  • The predicted excess was wrongly scaled with the ratio between run observation time and width of the time bin
  • The error was computed incorrectly

Furthermore, this PR leads to a huge speed-up by removing an uneccesary (and slow) loop over energy bins.

See also issue #1178 for more discussion.

@lmohrmann

This comment has been minimized.

Show comment
Hide comment
@lmohrmann

lmohrmann Oct 20, 2017

Contributor

I added another small fix: previously, the observed events were binned, bins outside of [spec.lo_threshold, spec.hi_threshold] were discarded and then the energy_range was applied. However, for the predicted excess, bins outside of the spectrum thresholds and energy_range are discarded. With the latest commit, this is also done for the observed excess.

It'd be good if someone could have a look at this and confirm that things make sense.

Contributor

lmohrmann commented Oct 20, 2017

I added another small fix: previously, the observed events were binned, bins outside of [spec.lo_threshold, spec.hi_threshold] were discarded and then the energy_range was applied. However, for the predicted excess, bins outside of the spectrum thresholds and energy_range are discarded. With the latest commit, this is also done for the observed excess.

It'd be good if someone could have a look at this and confirm that things make sense.

@cdeil cdeil added bug feature labels Oct 20, 2017

@cdeil cdeil added this to the 0.7 milestone Oct 20, 2017

@cdeil cdeil assigned jjlk and unassigned lmohrmann Oct 20, 2017

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Nov 3, 2017

Contributor

Hi @lmohrmann ,
Sorry for the delay, I was in holidays for two weeks. Thanks for the work! I do not have time to look at what you did right now, I'll be available next week. In the mean time let's merge and we'll do testing later. Okay for you?

Contributor

jjlk commented Nov 3, 2017

Hi @lmohrmann ,
Sorry for the delay, I was in holidays for two weeks. Thanks for the work! I do not have time to look at what you did right now, I'll be available next week. In the mean time let's merge and we'll do testing later. Okay for you?

@cdeil cdeil merged commit 5cb1772 into gammapy:master Nov 3, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@lmohrmann

This comment has been minimized.

Show comment
Hide comment
@lmohrmann

lmohrmann Nov 3, 2017

Contributor

Hi @jjlk, no problem. I see @cdeil already merged. It'd be good if you could have a look into this next week nevertheless, just to make sure what I did is not insane. Thanks in advance!

Contributor

lmohrmann commented Nov 3, 2017

Hi @jjlk, no problem. I see @cdeil already merged. It'd be good if you could have a look into this next week nevertheless, just to make sure what I did is not insane. Thanks in advance!

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 3, 2017

Member

It would be great to have a documented analysis that shows fluxes computed by Gammapy LC are correct. For HESS e.g. one could compute runwise fluxes for the 4 Crab runs from the test data release and check that integral fluxes in one or two energy bands roughly match the known spectrum. If someone does testing with CTA, please focus on the PKS 2155 runs, which were agreed on as the target for first checks (see https://forge.in2p3.fr/projects/data-challenge-1-dc-1/wiki/Comparison_of_ctools_and_Gammapy)

Member

cdeil commented Nov 3, 2017

It would be great to have a documented analysis that shows fluxes computed by Gammapy LC are correct. For HESS e.g. one could compute runwise fluxes for the 4 Crab runs from the test data release and check that integral fluxes in one or two energy bands roughly match the known spectrum. If someone does testing with CTA, please focus on the PKS 2155 runs, which were agreed on as the target for first checks (see https://forge.in2p3.fr/projects/data-challenge-1-dc-1/wiki/Comparison_of_ctools_and_Gammapy)

@lmohrmann lmohrmann deleted the lmohrmann:fix-lightcurve branch Nov 3, 2017

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