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 parameter auto scale #1696

Merged
merged 7 commits into from Aug 10, 2018

Conversation

Projects
2 participants
@cdeil
Member

cdeil commented Aug 10, 2018

This PR implements parameter auto-scaling before fitting to address the issue reported in #1531 and is one of the solutions discussed there.

@facero @robertazanin @bkhelifi @registerrier @cosimoNigro or anyone that is running analyses with Gammapy at the moment:

It should no longer be necessary to call model.parameters.set_parameter_errors. Now you should only set reasonable start values before the fit (e.g. spectral index of 2 and a flux norm of 1e-11 or 1e-12), and then run it.

Also: Please update to iminuit 1.3.2 that @HDembinski released today: http://iminuit.readthedocs.io/en/latest/changelog.html#august-5-2018
That fixes a bug in the covariance matrix reported by iminuit that is present in iminui 1.3.0 and 1.3.1 (but should not be there in iminui 1.2 or 1.3.2 or any later version).

In the few fits that we run have automated in Gammapy: they now all work without set_parameter_errors and results come out the same. Except for SpectrumFit.test_compound, there the fitted flux amplitude has changed. I'm not sure why, maybe the fit wasn't converging before and we asserted on arbitrary values. We should always add an assert on fit status in tests. I'll let this one slide and merge this now, so that people can start testing this.

Please report any issues you find soon, the Gammapy v0.8 release is planned for Monday!
(e.g. incorrect results, errors, non-converging fits, ...)

@cdeil cdeil added the feature label Aug 10, 2018

@cdeil cdeil added this to the 0.8 milestone Aug 10, 2018

@cdeil cdeil self-assigned this Aug 10, 2018

@cdeil cdeil added this to In progress in Modeling via automation Aug 10, 2018

@cdeil cdeil merged commit 7bb339c into gammapy:master Aug 10, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

Modeling automation moved this from In progress to Done Aug 10, 2018

@@ -283,23 +295,8 @@ def _ufloats(self):
upars = {}
for par, upar in zip(self.parameters, uarray):
upars[par.name] = upar
return upars
@property

This comment has been minimized.

@joleroi

joleroi Aug 13, 2018

Contributor

why did you remove this?

@joleroi

joleroi Aug 13, 2018

Contributor

why did you remove this?

This comment has been minimized.

@cdeil

cdeil Aug 14, 2018

Member

IMO it wasn't clear that these properties free and frozen are useful, so I removed them.
They are rarely needed, and in that case it's just one line.
Also the names aren't intuitive, here you returned the names of the free and frozen parameters, but naively I would have expected the list of thost Parameter objects returned.
So I thought about renaming to free_parameter_names, but then for now decided to just remove it.

@cdeil

cdeil Aug 14, 2018

Member

IMO it wasn't clear that these properties free and frozen are useful, so I removed them.
They are rarely needed, and in that case it's just one line.
Also the names aren't intuitive, here you returned the names of the free and frozen parameters, but naively I would have expected the list of thost Parameter objects returned.
So I thought about renaming to free_parameter_names, but then for now decided to just remove it.

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