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 energy bias computation #1115

Merged
merged 1 commit into from Sep 7, 2017

Conversation

Projects
None yet
4 participants
@cosimoNigro
Contributor

cosimoNigro commented Sep 5, 2017

Hi

there is a small mistake in the bias calculation for the energy dispersion, e_reco and e_true were inverted in the function EnergyDispersion.get_bias() w.r.t the definition http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDispersion.html

Thanks

@adonath adonath added the bug label Sep 5, 2017

@adonath adonath added this to the 0.7 milestone Sep 5, 2017

@joleroi

This comment has been minimized.

Show comment
Hide comment
@joleroi

joleroi Sep 5, 2017

Contributor

Thanks for spotting this. Let's wait if we have to update the tests.

Contributor

joleroi commented Sep 5, 2017

Thanks for spotting this. Let's wait if we have to update the tests.

@cdeil cdeil changed the title from bias definition corrected to Fix energy bias computation Sep 5, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 5, 2017

Member

@cosimoNigro - Thanks for the bugfix!

travis-ci fails are all completely unrelated and already fixed in master. I had a quick look at this locally. There is one test calling get_bias, but it's for a case where the bias is zero:
https://github.com/gammapy/gammapy/blob/master/gammapy/irf/tests/test_energy_dispersion.py#L31
and this function is used to generate the test case, and it doesn't have an option to set a bias:
http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDispersion.html#gammapy.irf.EnergyDispersion.from_gauss

There is this method which does allow to set a bias to generate a nice test case:
http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDispersion2D.html#gammapy.irf.EnergyDispersion2D.from_gauss

The proper fix here is:

  • Copy the EnergyDispersion2D.from_gauss implementation to EnergyDispersion.from_gauss, because EnergyDispersion2D.from_gauss is well-debugged by @registerrier during the Paris coding sprint in February.
  • Just loop over offset bins and call EnergyDispersion.from_gauss from EnergyDispersion2D.from_gauss to avoid the code duplication.
  • Update the test cases to have a case with small bias and check that it's computed OK.

So it's a bit of a larger project and will take 1-2 hours.

I'm -1 on just merging the fix without improving the implementation and tests for edisp.

@cosimoNigro or @joleroi - Does one of you have time to do it this week?
(If no, I could do it later this week and then merge.)

Member

cdeil commented Sep 5, 2017

@cosimoNigro - Thanks for the bugfix!

travis-ci fails are all completely unrelated and already fixed in master. I had a quick look at this locally. There is one test calling get_bias, but it's for a case where the bias is zero:
https://github.com/gammapy/gammapy/blob/master/gammapy/irf/tests/test_energy_dispersion.py#L31
and this function is used to generate the test case, and it doesn't have an option to set a bias:
http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDispersion.html#gammapy.irf.EnergyDispersion.from_gauss

There is this method which does allow to set a bias to generate a nice test case:
http://docs.gammapy.org/en/latest/api/gammapy.irf.EnergyDispersion2D.html#gammapy.irf.EnergyDispersion2D.from_gauss

The proper fix here is:

  • Copy the EnergyDispersion2D.from_gauss implementation to EnergyDispersion.from_gauss, because EnergyDispersion2D.from_gauss is well-debugged by @registerrier during the Paris coding sprint in February.
  • Just loop over offset bins and call EnergyDispersion.from_gauss from EnergyDispersion2D.from_gauss to avoid the code duplication.
  • Update the test cases to have a case with small bias and check that it's computed OK.

So it's a bit of a larger project and will take 1-2 hours.

I'm -1 on just merging the fix without improving the implementation and tests for edisp.

@cosimoNigro or @joleroi - Does one of you have time to do it this week?
(If no, I could do it later this week and then merge.)

@cosimoNigro

This comment has been minimized.

Show comment
Hide comment
@cosimoNigro

cosimoNigro Sep 5, 2017

Contributor

@cdeil I can try to take a look later, or tomorrow. If I need help I will come back to you.

Contributor

cosimoNigro commented Sep 5, 2017

@cdeil I can try to take a look later, or tomorrow. If I need help I will come back to you.

@cdeil cdeil assigned cdeil and unassigned joleroi Sep 7, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 7, 2017

Member

I'm merging this now, and will start a follow-up PR with the EDISP changes / improvements I mentioned above tomorrow. I prefer to first merge in all the open PRs to Gammapy to avoid merge conflicts.

Member

cdeil commented Sep 7, 2017

I'm merging this now, and will start a follow-up PR with the EDISP changes / improvements I mentioned above tomorrow. I prefer to first merge in all the open PRs to Gammapy to avoid merge conflicts.

@cdeil cdeil merged commit fc8767b into gammapy:master Sep 7, 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
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 7, 2017

Member

@cosimoNigro - Thanks for finding and fixing the issue!
I added you to the Gammapy contributor list just now: 5930feb

Member

cdeil commented Sep 7, 2017

@cosimoNigro - Thanks for finding and fixing the issue!
I added you to the Gammapy contributor list just now: 5930feb

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