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

Rewrite crab spectrum as class #777

Merged
merged 5 commits into from Nov 18, 2016

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Nov 16, 2016

This PR introduces a CrabSpectrum class, with the different reference models represented as SpectrumModel. This has the advantage, that all the methods of SpectrumModel, such as .integral()
and energy_flux() can be reused. It has now full Quantity support as well.

@adonath adonath added this to the 0.5 milestone Nov 16, 2016

@adonath adonath self-assigned this Nov 16, 2016

@cdeil

cdeil approved these changes Nov 17, 2016

Thanks!

I've left two inline comments.

Concerning API: I probably would have made this a factory function, and just attached whatever other info you want to the SpectralModel objects after construction. But if you prefer the CrabSpectrum class where one accesses the model all the time, OK.

In any case: add one or two examples how to call it to the docstring?

Show outdated Hide outdated gammapy/spectrum/models.py
Show outdated Hide outdated gammapy/spectrum/crab.py
Show outdated Hide outdated gammapy/spectrum/models.py
@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 17, 2016

Member

After thinking about this some more, I'm thinking maybe keeping it a class it a good idea?
If we want to add extra methods like "convert given flux to Crab units" we could add it there and the method docstring would show up in the docs.

Member

cdeil commented Nov 17, 2016

After thinking about this some more, I'm thinking maybe keeping it a class it a good idea?
If we want to add extra methods like "convert given flux to Crab units" we could add it there and the method docstring would show up in the docs.

@adonath

This comment has been minimized.

Show comment
Hide comment
@adonath

adonath Nov 17, 2016

Member

@cdeil The use case with the unit is what I had in mind as well. Right now the following works, which isn't too bad:

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

# define power law model
amplitude = 1E-12 * u.Unit('1 / (cm2 s TeV)')
index = 2.3
reference = 1 * u.TeV
pwl = PowerLaw(index, amplitude, reference)

# define crab reference
crab = CrabSpectrum('hess_pl').model

# compute flux at 10 TeV in crab units
energy = 10 * u.TeV
flux_cu = (pwl(energy) / crab(energy)).to('%')
print(flux_cu)

# compute integral flux in crab units
emin, emax = [1, 10] * u.TeV
flux_int_cu = (pwl.integral(emin, emax) / crab.integral(emin, emax)).to('%')
print(flux_int_cu)

I think it's hard to implement a better API, because the flux unit is parametric and depends on what you do with the spectrum you want to measure the flux in crab units for...so something along the lines of:

pwl(energy).to('crab')
pwl.integral(emin, emax).to('crab')

Is very hard to implement and probably not worth the effort.

Possibly one could have something like:

pwl_cu = CrabSpectrum('meyer').use_as_unit_for(pwl)
pwl_cu(energy)

But it's not a super nice API either...

Member

adonath commented Nov 17, 2016

@cdeil The use case with the unit is what I had in mind as well. Right now the following works, which isn't too bad:

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

# define power law model
amplitude = 1E-12 * u.Unit('1 / (cm2 s TeV)')
index = 2.3
reference = 1 * u.TeV
pwl = PowerLaw(index, amplitude, reference)

# define crab reference
crab = CrabSpectrum('hess_pl').model

# compute flux at 10 TeV in crab units
energy = 10 * u.TeV
flux_cu = (pwl(energy) / crab(energy)).to('%')
print(flux_cu)

# compute integral flux in crab units
emin, emax = [1, 10] * u.TeV
flux_int_cu = (pwl.integral(emin, emax) / crab.integral(emin, emax)).to('%')
print(flux_int_cu)

I think it's hard to implement a better API, because the flux unit is parametric and depends on what you do with the spectrum you want to measure the flux in crab units for...so something along the lines of:

pwl(energy).to('crab')
pwl.integral(emin, emax).to('crab')

Is very hard to implement and probably not worth the effort.

Possibly one could have something like:

pwl_cu = CrabSpectrum('meyer').use_as_unit_for(pwl)
pwl_cu(energy)

But it's not a super nice API either...

@cdeil

This comment has been minimized.

Show comment
Hide comment
@cdeil

cdeil Nov 17, 2016

Member

I think what you have now, explaining how to do it via the docs, is a good solution.
Merge when travis-ci passes, right?

Member

cdeil commented Nov 17, 2016

I think what you have now, explaining how to do it via the docs, is a good solution.
Merge when travis-ci passes, right?

@adonath adonath merged commit d171ecf into gammapy:master Nov 18, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adonath adonath deleted the adonath:rewrite_crab_spectrum_Class branch Nov 18, 2016

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