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 MAGIC Crab reference spectrum #1134

Merged
merged 7 commits into from Sep 15, 2017

Conversation

Projects
None yet
3 participants
@cosimoNigro
Contributor

cosimoNigro commented Sep 14, 2017

Hi I took the liberty to add the latest Crab spectra measured by MAGIC in gammapy/spectrum/crab.py. The reference is
http://adsabs.harvard.edu/abs/2015JHEAp...5...30A

I didn't modify gammapy/spectrum/tests/test_crab.py because it is not clear to me how the numbers hardcoded at the beginning of the macro in CRAB_SPECTRA for the assert_quantity_allclose test are calculated. Could you modify it accordingly or explaining how to do, before merging?

Thanks

@adonath adonath self-assigned this Sep 14, 2017

@adonath adonath added the feature label Sep 14, 2017

@adonath adonath added this to the 0.8 milestone Sep 14, 2017

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Sep 14, 2017

Member

@cosimoNigro Thanks for this PR, the changes look good to me!

It would be still great if you could add the test numbers for the spectra you added. The numbers in gammapy/spectrum/tests/test_crab.py are the differential flux at 2 TeV ('dnde'), the integral flux above 1 TeV ('flux') and the local spectral index at 2 TeV ('index'). You can compute the numbers as follows:

from gammapy.spectrum import CrabSpectrum
from astropy import units as u

energy = 2 * u.TeV
emin, emax = [1, 1e3] * u.TeV

crab_spectrum = CrabSpectrum('magic_lp')

print(crab_spectrum.model(energy))
print(crab_spectrum.model.integral(emin, emax))
print(crab_spectrum.model.spectral_index(energy))

Just copy and paste the resulting numbers as a dict into the CRAB_SPECTRA list, like the others, that are already there. Does that make sense?

Member

adonath commented Sep 14, 2017

@cosimoNigro Thanks for this PR, the changes look good to me!

It would be still great if you could add the test numbers for the spectra you added. The numbers in gammapy/spectrum/tests/test_crab.py are the differential flux at 2 TeV ('dnde'), the integral flux above 1 TeV ('flux') and the local spectral index at 2 TeV ('index'). You can compute the numbers as follows:

from gammapy.spectrum import CrabSpectrum
from astropy import units as u

energy = 2 * u.TeV
emin, emax = [1, 1e3] * u.TeV

crab_spectrum = CrabSpectrum('magic_lp')

print(crab_spectrum.model(energy))
print(crab_spectrum.model.integral(emin, emax))
print(crab_spectrum.model.spectral_index(energy))

Just copy and paste the resulting numbers as a dict into the CRAB_SPECTRA list, like the others, that are already there. Does that make sense?

@cosimoNigro

This comment has been minimized.

Show comment
Hide comment
@cosimoNigro

cosimoNigro Sep 14, 2017

Contributor

Hi @adonath, thanks for the tip, I also though that I should directly calculate the values, but wanted to be sure, I am new to all this pytest-ing :D.
I modified gammapy/spectrum/tests/test_crab.py and checked the tests with
python -m pytest -v gammapy/spectrum/tests/test_crab.py

Contributor

cosimoNigro commented Sep 14, 2017

Hi @adonath, thanks for the tip, I also though that I should directly calculate the values, but wanted to be sure, I am new to all this pytest-ing :D.
I modified gammapy/spectrum/tests/test_crab.py and checked the tests with
python -m pytest -v gammapy/spectrum/tests/test_crab.py

@cdeil cdeil modified the milestones: 0.8, 0.7 Sep 14, 2017

@cdeil

cdeil approved these changes Sep 14, 2017

Code looks good to me. @adonath - Could you please check the model parametrisation and values against the given paper before merging?

@adonath adonath self-requested a review Sep 15, 2017

@adonath

I cross-checked the numbers and parametrization of the models and they look correct to me. The Travis-CI fails are all unrelated. This PR is ready to merge.

@adonath adonath merged commit 2f916bf into gammapy:master Sep 15, 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
@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Sep 15, 2017

Member

@cosimoNigro Thanks again for this PR! One minor comment for future contributions (which are very welcome!): maybe you could try to put a little bit more explicit commit messages e.g. instead of 'checked', something like 'Add table and formula references to code comment in crab.py'

Member

adonath commented Sep 15, 2017

@cosimoNigro Thanks again for this PR! One minor comment for future contributions (which are very welcome!): maybe you could try to put a little bit more explicit commit messages e.g. instead of 'checked', something like 'Add table and formula references to code comment in crab.py'

@cdeil cdeil changed the title from Added MAGIC Crab spectra to crab.py to Add MAGIC Crab reference spectrum Nov 6, 2017

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