Skip to content
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 flux point computation method #679

merged 4 commits into from Aug 22, 2016

Add flux point computation method #679

merged 4 commits into from Aug 22, 2016


Copy link

@joleroi joleroi commented Aug 18, 2016

This PR adds

  • DifferentialFluxPoints.compute
  • A helper function to compute flux points with a minimum significance (similar to FitSpectrum)
  • Example in the docs
@joleroi joleroi added this to the 0.5 milestone Aug 18, 2016
@joleroi joleroi self-assigned this Aug 18, 2016
@joleroi joleroi force-pushed the joleroi:flux_points branch from 08cd30c to f4bffde Aug 18, 2016
@joleroi joleroi force-pushed the joleroi:flux_points branch from 940c21e to c2bced7 Aug 18, 2016
Copy link

@cdeil cdeil commented Aug 18, 2016

@joleroi - Thanks!

In the tutorial, I don't get why you basically show the same example twice.
You mention that the difference is whether the global fit was done, but you do it in both cases, no?
It looks to me like you just do it via fit.compute_fluxpoints once, and once via DifferentialFluxPoints.compute.

We have to think about what a good code organisation and API is for 1D spectral analysis.

My first guess would be that something like
would be good, i.e. change the name SpectrumFit to SpectrumAnalysis and expose one or several methods to compute flux points on that object (always assuming the global model is available).

The code to compute one flux point will get complex (e.g. root finding to get Rolke limits, or calling into iminuit.minos, ...). At the moment the flux point computation is bin by bin, independently. So maybe put the computation of one flux point in a separate function? (doesn't have to be exposed in the end-user API, but I think this is simpler and better testable code?)

I didn't have a close look at this PR, and I'm not sure my suggestions are good ones or not.

Feel free to merge any time!
If it works it's a big step forward and people (including me) will only start using it once it's in master.

Copy link
Contributor Author

@joleroi joleroi commented Aug 22, 2016

@cdeil the basic idea was to make the flux point computation independent of the spectral fit. That's why it's on DifferentialFluxPoints.compute (There might be a better place but at the moment it's fine there IMO). There are already now use cases where people might want to compute flux points without having to setup a fit (e.g. HAP export). The method on SpectralFit is just a convenience method if you want to compute points after having done a global fit (which is probably the most common use case).

The thing that annoys me the most with FitSpectrum is that's is setup to do exactly one thing and it's impossible (or very hard) to get access to the individual components. So I am +10 to leave the separation between analysis steps (extraction, global fit, flux points) as it is.

@joleroi joleroi merged commit 700f636 into gammapy:master Aug 22, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
@joleroi joleroi deleted the joleroi:flux_points branch Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants