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

Sync sky model parameters with components #1556

Merged
merged 4 commits into from Jul 25, 2018

Conversation

Projects
2 participants
@cdeil
Copy link
Member

cdeil commented Jul 24, 2018

I tried to update analysis_3d.ipynb just now to add a SkyModel fit, and noticed a few issues.

There doesn't seem to be a way to get a SkyModel with fitted parameters back, e.g. print(fit.model) shows the start values even though the fit ran and fit.model.parameters actually contains the result.

The core of the problem is that SkyModel makes a new ParameterList that is not synced with the one from the components.
Minimal example:

from gammapy.image.models import SkyGaussian
from gammapy.spectrum.models import PowerLaw
from gammapy.cube.models import SkyModel
spatial_model = SkyGaussian(lon_0='0 deg', lat_0='0 deg', sigma='1 deg')
spectral_model = PowerLaw()
model = SkyModel(spatial_model, spectral_model)
model.spatial_model.parameters.parameters[0].value = 42
print(model.parameters['lon_0'])  # changed value: 42 deg
print(model.spatial_model.parameters['lon_0']) # initial value: 0 deg

There's also doesn't seem to be an API yet on ParameterList to set parameter values, one has to reach into the parameter_list.parameters internals:

for parval, par in zip(p, self.parameters.parameters):

I will try to make this better today and in the next days, but @joleroi or @adonath - if you have time to help and pair-code on this with me, it would be very helpful.

@cdeil cdeil added the bug label Jul 23, 2018

@cdeil cdeil added this to the 0.8 milestone Jul 23, 2018

@cdeil cdeil self-assigned this Jul 23, 2018

@cdeil cdeil added this to To do in Modeling via automation Jul 23, 2018

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Jul 23, 2018

Oh boy, zero active tests for the current Parameter and ParameterList:
https://github.com/gammapy/gammapy/blob/master/gammapy/utils/tests/test_modeling.py

I'll make modeling / fitting my focus this week, but today I didn't get to it.

@joleroi

This comment has been minimized.

Copy link
Contributor

joleroi commented Jul 23, 2018

Oh boy, zero active tests for the current Parameter and ParameterList:

These are old tests and can probably be removed. XML I/O is currently tested here

Also note the API to set individual parameters

model.parameters['lon_0'].value = 0.2

or

model.parameters['lon_0'].quantitiy = 0.2 * u.deg
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Jul 23, 2018

I'll remove those tests and put a few new ones, unit tests for Parameter and ParameterList, tomorrow to establish some behaviour and familiarise myself with them.

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Jul 24, 2018

@registerrier @joleroi or @adonath - could you please have a look at 95e46f4 ?

I think the CompoundModel already has a working example of syncing parameters to two parts?
The trick is simply to have different lists of parameters, and have those lists point to the same Parameter objects. I had a look at Sherpa, that's also how they do it.

But I have this failing test:
https://gist.github.com/cdeil/0656f59e21842c3a2e594a8355b5bbc5
I don't understand why Minuit fits successfully, but the fit result parameters are not in the model after, it's still at the initial values!?

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Jul 24, 2018

I see:

The input `~gammapy.utils.modeling.ParameterList` is copied internally
before the fit and will thus not be modified. The best-fit parameter values
are contained in the output `~gammapy.utils.modeling.ParameterList` or the
`~iminuit.Minuit` object.

and also the code at

parameters = parameters.copy()

and below.

I don't think it's possible to work like this. The CompoundSkyModel and the factorised SkyModel need their Parameter objects updated during the likelihood fit. Or we need to change the evaluate to be static methods where parameters are also passed like in Astropy.

Thoughts?

(I'll try to change the iminuit interface now)

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Jul 24, 2018

This seems to do the trick: 969c824

@registerrier - OK to go that direction?

@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Jul 25, 2018

I'm going ahead, merging this now.

So we now have SkyModel, SumSkyModel and CompoundSkyModel all with the same strategy to hold the component models as data members, and to assemble and disassemble the parameters in the parameters getter and setter. I.e. the components own the parameters, the composite classes provide a view. During fitting those parameters change in place and are then accessed from evaluate.

@joleroi @registerrier @adonath - There's several things here I'd like to discuss, at least:

  • Merge SourceLibrary and SumSkyModel into one class - called SkyModels? (not sure)
  • Rename SkyModel to FactorisedSkyModel (or some better name?)
  • Introduce an ABC SkyModel? Does it implement __call__ like we have for the spectral and spatial models?
  • Is it OK that during optimisation we go through Parameter objects? I think Astropy avoids this? What about Sherpa? Where to we use parameter units and quantities in the evaluation?
  • Location of these model classes in Gammapy?

If you have thoughts, please comment here or make issues with suggestions, so that we can make some progress this week. And then we can talk in the Gammapy call on Friday.

@cdeil cdeil merged commit 59ea847 into gammapy:master Jul 25, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

Modeling automation moved this from To do to Done Jul 25, 2018

@cdeil cdeil changed the title SkyModel parameter sync with components Sync sky model parameters with components Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.