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

Unify API of Gammapy Fit classes #1785

Merged
merged 16 commits into from Sep 13, 2018

Conversation

Projects
3 participants
@adonath
Member

adonath commented Sep 12, 2018

This PR unifies the current Gammapy ...Fit classes by making them inherit from a Fit base class, introduced in utils/fitting/base.py. The Fit base class implements a .run() and .fit() method, defining the main API. The sub-classes have to implement a .total_stat() method, that define the total likelihood to be minimized by the optimizer. Currently the .fit() and .run() method return a dict object with a little bit of info about the fit as well as the best-fit model.

This PR also makes a few minor changes to the fit_minuit() method so that activating the sherpa back-end in a future PR becomes easier.

@adonath adonath added the cleanup label Sep 12, 2018

@adonath adonath added this to the 0.8 milestone Sep 12, 2018

@adonath adonath self-assigned this Sep 12, 2018

@adonath adonath added this to In progress in Modeling via automation Sep 12, 2018

@cdeil

cdeil approved these changes Sep 12, 2018

@adonath - This looks great, thank you!

Suggest to change the key best-fit-model to just model in the results object? The other is a bit long to remember and type.

The other question I have is if we need both stat and total_stat, and whether the use is consistent. So stat is per bin, and total_stat is the sum, possibly taking a mask into account? Maybe stat_sum would be clearer? What names dis Sherpa choose? If you don't have time to look at this, OK to leave as-is.

There's one link in one notebook that needs to be updated with the rename:
There's
https://travis-ci.org/gammapy/gammapy/jobs/427775619#L2299

@adonath - Please merge this when you can tomorrow, so that we can continue testing and using this a bit this week.

@@ -214,10 +213,10 @@ def test_values(self):
assert_allclose(actual, 6.3298e-11, rtol=1e-2)
actual = flux_points.table["dnde_errn"][0]
assert_allclose(actual, 5.9315e-12, rtol=1e-2)
assert_allclose(actual, np.nan, rtol=1e-2)

This comment has been minimized.

@cdeil

cdeil Sep 12, 2018

Member

Do you understand why you get "np.nan" in this test now? Then OK, no need to explain.

This comment has been minimized.

@adonath

adonath Sep 13, 2018

Member

No, unfortunately I don't understand why this changed. Only the upper limit computation was affected, which seems to be buggy anyway (see #1635). The problem is that the FluxPointEstimator is probably the worst piece of code ever written in Gammapy (50% by me...:-) and should be reworked and simplified as soon as possible. Currently the UL computation uses a custom method with brentq and we should just reuse Minuit...

This comment has been minimized.

@cdeil

cdeil Sep 13, 2018

Member

Then I'd suggest to leave a TODO comment line with a link to the commit or PR where this changed (or now to this comment directly), to help the person that will work on the FluxPointEstimator next see what the values previously were.

"""
@abc.abstractmethod
def total_stat(self, parameters):

This comment has been minimized.

@cdeil

cdeil Sep 12, 2018

Member

Does it make sense to pass parameters to total_stat?
Why not access them from self._model.parameters from this method?

This comment has been minimized.

@adonath

adonath Sep 13, 2018

Member

Right now total_stat() is directly used as the call-back function for the optimizers, so it is needed to pass in the parameters. For cleaner API I thought about making a private _total_stat, which is used as the call-back and then introducing a public property .total_stat.

This comment has been minimized.

@joleroi

joleroi Sep 13, 2018

Contributor

I like having total_stat as a public method accepting parameters, because this is what is passed to the optimizers. Also it's useful to be able to calculated the Likelihood for a given set of model parameters without having to know about self._model e.g. to compute likelihood profiles

@joleroi

This comment has been minimized.

Contributor

joleroi commented Sep 13, 2018

Maybe stat_sum would be clearer? What names dis Sherpa choose?

Sherpa uses statval and fvec which I don't like at all. I like total_stat but if you agree to get rid of it, maybe stat_per_bin and (statval or stat)

@cdeil

This comment has been minimized.

Member

cdeil commented Sep 13, 2018

@adonath and I are discussing offline. Merging this now. Will continue a bit in follow-up commits / PRs.

@cdeil cdeil merged commit 95f52b6 into gammapy:master Sep 13, 2018

1 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
Scrutinizer Analysis: Errored – Tests: failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Modeling automation moved this from In progress to Done Sep 13, 2018

@adonath adonath referenced this pull request Sep 13, 2018

Merged

Further improve Fit class #1789

@cdeil cdeil referenced this pull request Sep 21, 2018

Closed

SpectrumFit run and write #1418

@adonath adonath deleted the adonath:issue_#1783 branch Nov 20, 2018

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