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 3FHL spectral indexes for PowerLaw model #1127

Merged
merged 1 commit into from Sep 14, 2017

Conversation

Projects
None yet
2 participants
@jjlk
Contributor

jjlk commented Sep 11, 2017

Hi @cdeil , @adonath ,
This PR fix the 3FHL spectral indexes for the sources modeled with a PowerLaw.

@cdeil : I didn't find anything wrong in the print output

Oké to merge?

@jjlk jjlk requested review from cdeil and adonath Sep 11, 2017

@jjlk

This comment has been minimized.

Show comment
Hide comment
@jjlk

jjlk Sep 11, 2017

Contributor

Not oké to merge, I have a problem with git. I'll go back to you

Contributor

jjlk commented Sep 11, 2017

Not oké to merge, I have a problem with git. I'll go back to you

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 11, 2017

Member

@jjlk - I think in the printout there's the same mistake for 3FHL, no?
https://github.com/gammapy/gammapy/pull/1127/files#diff-99130bfaec86b7c20f2213209a553ee0R700
It always prints the Spectral_Index field, which is not the right value for the PL case.

I think the corresponding 3FGL or GammaCat printout function is done better, putting the whole printout for spectral model in if / else block, not trying to combine the cases, which IMO makes the code harder to understand. So in this case I prefer completely independent if / else blocks for the spectral model printout, even if it means duplicating a few lines of code.

Member

cdeil commented Sep 11, 2017

@jjlk - I think in the printout there's the same mistake for 3FHL, no?
https://github.com/gammapy/gammapy/pull/1127/files#diff-99130bfaec86b7c20f2213209a553ee0R700
It always prints the Spectral_Index field, which is not the right value for the PL case.

I think the corresponding 3FGL or GammaCat printout function is done better, putting the whole printout for spectral model in if / else block, not trying to combine the cases, which IMO makes the code harder to understand. So in this case I prefer completely independent if / else blocks for the spectral model printout, even if it means duplicating a few lines of code.

@cdeil cdeil self-assigned this Sep 11, 2017

@cdeil cdeil added the bug label Sep 11, 2017

@cdeil cdeil added this to the 0.7 milestone Sep 11, 2017

@cdeil cdeil removed the request for review from adonath Sep 14, 2017

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 14, 2017

Member

I think if I start editing here there will be merge conflicts. So I'm merging this now, and will make a follow-up commit in master, mostly to improve tests. (note how the change here didn't require a change in the tests, the tests didn't cover spectral index)

@jjlk - Félicitations pour avoir trouvé et réparé ce bug!

Member

cdeil commented Sep 14, 2017

I think if I start editing here there will be merge conflicts. So I'm merging this now, and will make a follow-up commit in master, mostly to improve tests. (note how the change here didn't require a change in the tests, the tests didn't cover spectral index)

@jjlk - Félicitations pour avoir trouvé et réparé ce bug!

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

Member

Correction: actually TestFermi3FHLObject.test_spectral_model[352-PowerLaw-desired0] did fail. I'll adjust the assert and do other improvements to the tests in master.

Member

cdeil commented Sep 14, 2017

Correction: actually TestFermi3FHLObject.test_spectral_model[352-PowerLaw-desired0] did fail. I'll adjust the assert and do other improvements to the tests in master.

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Sep 14, 2017

Member

I've tried to improve the Fermi source printout code a bit in 8b7ab13 and the tests in 567a0a0 .

I think the tests are a bit better now (easier to read and extend). For the 3FHL spectral info printout, I checked the two sources that are also used in the test, I think it's better now. For the 3FGL spectral info printout, I didn't really manage to make it better, the code is very hard to read / maintain as-is, but probably correct for all cases, and quite hard to write in a better way. Moving on ... of course, future issue report or PRs to improve things welcome any time.

Member

cdeil commented Sep 14, 2017

I've tried to improve the Fermi source printout code a bit in 8b7ab13 and the tests in 567a0a0 .

I think the tests are a bit better now (easier to read and extend). For the 3FHL spectral info printout, I checked the two sources that are also used in the test, I think it's better now. For the 3FGL spectral info printout, I didn't really manage to make it better, the code is very hard to read / maintain as-is, but probably correct for all cases, and quite hard to write in a better way. Moving on ... of course, future issue report or PRs to improve things welcome any time.

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