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 spectrum energy flux computation #591

Merged
merged 4 commits into from Jun 29, 2016

Conversation

Projects
None yet
2 participants
@adonath
Member

adonath commented Jun 27, 2016

This PR adds an integrate_loglog helper function, which is a thin wrapper around Naima's trapz_loglog to handle the integration min / max with units.
Furthermore this PR implements a power_law_energy_flux function to compute energy fluxes from integral flux in a certain energy range.

@cdeil cdeil added the feature label Jun 27, 2016

@cdeil cdeil added this to the 0.5 milestone Jun 27, 2016

@cdeil cdeil self-assigned this Jun 27, 2016

@@ -101,6 +102,37 @@ def power_law_flux(I=1, g=g_DEFAULT, e=1, e1=1, e2=E_INF):
return I / _conversion_factor(g, e, e1, e2)
def power_law_energy_flux(I, g=g_DEFAULT, e=1, e1=1, e2=10):
"""

This comment has been minimized.

@cdeil

cdeil Jun 27, 2016

Member

For docstrings with math formulae, I suggest we always use "raw literal strings" (see here) to avoid having to write \\ in the formula.
See example here
I think we do this everywhere, if not I think we should.

"""
Test energy flux computation for power law against numerical solution.
"""
from naima.utils import trapz_loglog

This comment has been minimized.

@cdeil

cdeil Jun 27, 2016

Member

This import can be removed in this test, no?

from naima.utils import trapz_loglog
@cdeil

This comment has been minimized.

Member

cdeil commented Jun 27, 2016

@adonath - Thanks!

I would suggest to rename integrate_loglog to spectrum_integrate.

If I see it correctly integrate_loglog_unc is almost the same as integrate_loglog, except that it uses unumpy and can do error propagation?
Maybe expose that version via an option use_unumpy=False instead of a second parallel function?

Please add a test, especially for integrate_loglog_unc that exercises the code and asserts on the numerical result.

I'm not super happy about introducing the dependency on Naima in Gammapy just for trapz_loglog, especially since it has to be monkey-patched to work with unumpy. I'm not sure what's the best way to handle this, we never found a good solution and thought it through for whether Gammapy should depend on Naima or the other way around. (FYI: Gammapy only has Numpy and Astropy as required dependencies.) @zblz - Thoughts?

f = lambda x: x * power_law_evaluate(x, norm, g, e)
val = integrate_loglog(f, e1, e2)
assert_quantity_allclose(val, eflux)

This comment has been minimized.

@cdeil

cdeil Jun 27, 2016

Member

Please add an assert on the computed result (i.e. the number).
If there's a bug or someone introduces one in the future in integrate_loglog, we won't notice with this test.

I would prefer to remove the little re-implementation of energy flux computation in this test function,
but if you prefer, OK to keep in addition to the assert on the number.

@cdeil cdeil referenced this pull request Jun 27, 2016

Closed

Clean up spectrum code #33

@adonath adonath force-pushed the adonath:energy_flux_computation branch from e6d7e23 to 0192954 Jun 28, 2016

@cdeil cdeil changed the title from Energy flux computation to Add spectrum energy flux computation Jun 29, 2016

@cdeil cdeil merged commit c8bee01 into gammapy:master Jun 29, 2016

2 checks passed

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

This comment has been minimized.

Member

cdeil commented Jun 29, 2016

Thanks!

@adonath adonath deleted the adonath:energy_flux_computation branch Nov 20, 2018

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