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 fitter class #979

Merged
merged 8 commits into from Apr 26, 2017
Merged

Conversation

@adonath
Copy link
Member

@adonath adonath commented Apr 19, 2017

This PR adds a FluxPointFitter class to fit a common model to a given set of flux points. A demo notebook is shown here.

TODO:

  • Rework flux point upper limit handling
  • Improve test (add asserts on more values of the result dict)
  • Wait for #978 to be merged and rebase
Copy link
Member

@cdeil cdeil left a comment

@adonath - Thanks!

I've left some inline comments.

Concerning your task list:

Rework flux point upper limit handling

Can you merge this ASAP and defer that to the future?
For now, how about adding a two-line FluxPoints.drop_ul classmethod and in the fitter to assume it's all non-UL?

Improve test (add asserts on more values of the result dict)

Yes, please add asserts on the other numbers in the results dict and the parameter errors.

And can you expose the Sherpa result objects? I.e. give people access to what Sherpa has built in without needing to re-expose a different set of results.

Wait for #978 to be merged and rebase

It's unrelated. But I'll merge that within the hour.

@@ -670,3 +670,167 @@ def plot(self, ax=None):
ax = plt.gca()

# TODO


def chi2_flux_points(flux_points_list, models):

This comment has been minimized.

@cdeil

cdeil Apr 19, 2017
Member

Why pass as list of models and then only access the first model? -> suggest to just pass a model.

Is it really useful to have lists of flux points? -> suggest to create a two-line FluxPoints.stack helper function that takes a list of flux points and stacks them, calling astropy.table.Table.vstack and to show a docs example how to do that.

----------
optimizer : ['simplex', 'moncar', 'gridsearch']
Which optmizer to use.

This comment has been minimized.

@cdeil

cdeil Apr 19, 2017
Member

error_estimator option not documented.

filename = '$GAMMAPY_EXTRA/test_datasets/spectrum/flux_points/flux_points.fits'
flux_points = FluxPoints.read(filename)
fitter = FluxPointsFitter([flux_points])

This comment has been minimized.

@cdeil

cdeil Apr 19, 2017
Member

I don't think this example works any more. API changed.
Suggest to remove it and to point to the high-level docs for an example.

@cdeil
Copy link
Member

@cdeil cdeil commented Apr 19, 2017

The notebook is nice!

Suggest to add an intro at the top what it is, and to also print out the fitted model results.

Why not keep it a bit simpler and use the same flux points for the three spectral models?
The last one is best, with the weak signal, where parameter errors become visible.

Also I think it might be simpler to just pass data and model to the fitter init, and use one fitter per fit and to keep it simple.
At the moment you keep passing data and model in to different methods, and there's not really much gain (but some extra complexity and possibility for bugs by getting into a bad state after using it for a while for multiple fits) from re-using fitters, the setup is cheap. This is also how Sherpa and Minuit do it, no?

@cdeil cdeil added the feature label Apr 19, 2017
@cdeil cdeil added this to the 0.6 milestone Apr 19, 2017
@adonath adonath force-pushed the adonath:add_flux_point_fitter branch 2 times, most recently from f3f89e0 to 63e7532 Apr 25, 2017
@adonath adonath force-pushed the adonath:add_flux_point_fitter branch from 63e7532 to 14d76b0 Apr 26, 2017
@cdeil
Copy link
Member

@cdeil cdeil commented Apr 26, 2017

@adonath - Merge?

@adonath adonath merged commit 45b23dd into gammapy:master Apr 26, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@cdeil
Copy link
Member

@cdeil cdeil commented Apr 27, 2017

Some small fixes and cleanup in 16a2df3 .

@adonath adonath deleted the adonath:add_flux_point_fitter branch Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants