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

PIG 7 - Models #1971

Merged
merged 2 commits into from Oct 18, 2019
Merged

PIG 7 - Models #1971

merged 2 commits into from Oct 18, 2019

Conversation

@adonath
Copy link
Member

@adonath adonath commented Dec 20, 2018

This PR adds PIG 7: add proposal and work plan for improving the current model framework of Gammapy and bring it in a state which is acceptable for the Gammapy v1.0 release.

I plan to open a GitHub project for this PIG with associated issues for the list of proposed PRs at the end of the PIG.

To-do list:

  • Add section about adding new parametric models (e.g. elliptical spatial models)
  • Add section about .evaluate() and Model.__call__
  • Add section about parameter linking
@adonath adonath added the pig label Dec 20, 2018
@adonath adonath added this to the 0.11 milestone Dec 20, 2018
@adonath adonath self-assigned this Dec 20, 2018
@adonath adonath added this to In progress in gammapy.modeling via automation Dec 20, 2018
@adonath adonath requested review from cdeil, registerrier and AtreyeeS Dec 20, 2018
@adonath
Copy link
Member Author

@adonath adonath commented Dec 20, 2018

One important thing I just discussed with @cdeil: we should add an API proposal to this PIG how to handle linked parameters.

Copy link
Member

@cdeil cdeil left a comment

@adonath - Thank you for the writeup!

I think it will be very useful to find good solutions and then to implement this step by step in early 2019.

Clarifying the following points is very important, something should be said in the PIG:

  • How does this compare to existing solutions? (Astropy, Sherpa, 3ML, ctools?) Do we continue rolling our own or should using astropy.modeling be investigated? This question came up many times, and I think putting some info / answer in this PIG to document the decision would be useful.
  • The proposal doesn’t mention __call__ or evaluate and the question of when to call which one and what their signatures are. Similarly for __init__ there’s the question how it works. What about quantity handling? What about model evaluation caching? The two main examples we’re looking at: Sherpa and Astropy, are very different how they work.
  • What about linked parameters and priors?

Question:

  • What about 1D spectral analysis? I think you’re missing 1D spectrum background model classes, if we want to continue to support 1D spectrum analysis, and do it in a similar way as we do for map-based analysis, no?

Probably to be discussed a bit later:

  • You say “IRF should be part of the model” and propose to add new classes that combine models and IRF as new model classes. But you never say why this is useful. As you know, I’m not convinced it’s useful to call this a model. It’s probably best to discuss this when also a PIG for datasets that explains how model evaluation is orchestrated is up. For now, you could add a TODO comment saying that a link should be added to the datasets PIG from this one.

Smaller comments inline.

characteristics, such as PSF, energy resolution, effective area and background
are also considered as part of the model framework. Models are used both in interactive analysis,
as well as scripted or command line based analyses. For this reason they must
implement an easy to use API as well as serialization to XML and YAML data formats.

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

"must have XML and YAML" -> "must have model serialisation"?

the need is unclear are marked with a question mark.

Parametric models are named after their parametrization, such as `Powerlaw` or
`Gaussian`. In case the name is not unique (e.g. for a gaussian spectral and spatial

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

Probably it's best to always add the "Spatial" and "Spectral" prefix?

Adding it sometimes will probably lead to confusion, or actual problems, e.g. if we only realise later that we need a SpatialSpam in addition to a spectral spam, and have to rename Spam to SpectralSpam.

Probably the Python class names will not be identical to the model name strings in the serialisation language?
E.g. SpectrumPowerLaw might be {'type': 'powerlaw'} in the spectrum section of the model specification.
I think it might be very common to create models from Python scripts by writing such model specification dicts, i.e. to basically use the human-friendly model serialisation scheme, and then to call a factory function. If this is available, the need for short Model class names to save typing is reduced.

Maybe even listing proposed names for all models would be a good idea, then it's much easier to see what we'll get. Note that there were several previous attempts at this, e.g. open-gamma-ray-astro/gamma-astro-data-formats#41 or https://github.com/gammapy/gamma-cat/tree/master/model_serialisation .

Discussing / Deciding the names will take up quite a bit of space in an already super-long PIG . So if you don't add it here, I would suggest you link to another place where this is to be figured out. But we need to figure it out anyways, so we could also do it here. Note that a PIG can come with an extra file, so you could just add that here to keep the PIG RST short. Or if you don't want to discuss names here, explicitly state in the PIG that this will come later. But my comment concerning sometimes adding the prefix and sometimes not stands, I don't think that's a good idea.

`SpatialGaussian` or `SpatialConstant` and `SpectralConstant`).

In addition there is the `SourceModels` class to represent a sum of `SourceModel`
objects.

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

You ave only talking about "SourceModels" for the cases where there are multiple source models.
Maybe it could also make sense to have multiple background models?
So should there be a "BackgroundModels"?
Or just a "Models" as one list where source and background is listed?

I'm not sure what the best solution is, but I'd suggest to expand this sentence a bit to make sure this has been considered in the design.

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

I think for v1.0 it's probably not needed to support multiple (hadronic) background models. Technically astrophysical background models (IEM) are already supported via the SourceTemplate class. Long-term I agree it might make sense to introduce support for multiple background models. I'll re-think this part to see if there is a need for such an abstraction now, if not I'll add a sentence to explain this.

Introduction of background models
---------------------------------
For any kind of IACTs analysis hadronic background models are required, which model
the residual hadronic (gamma-like) background emission from templates. They differ

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

Suggest to remove "from templates". Background models could also be parametric. (e.g. ctools has some of those already)

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

Added a sentence that background models can be parametric as well.

~~~~~~~~~~~~~~~~~
Base class for all background models.

`BackgroundIRFModel`

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

You are only considering background models for maps. Don't we need background models for 1D spectra as well?

-------------------------------

We propose to introduce a `gammapy.models` or `gammapy.modeling` sub-module, which
bundles all the model classes in single namespace.

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

Do you mean to only expose models in gammapy.models?
Or to also expose them where they are now, and to expose them via a second namespace in addition?
Please clarify?

Also, I think there should be a minimal answer of 1-2 sentences on "why"? This is a major decision concerning code organisation in Gammapy. It might be best to just defer this question to a later PIG and simply leave as-is for now?
(and not discuss / decide whether to introduce gammapy.models) since it's not needed to build the features?

Like I said earlier: I assume that even from Python scripts, the most common way to create models will be to call a factory function, passing a "model spec" string or dict as input, making the question of model class names and namespace locations less important.

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

I removed the suggestion to introduce a separate name space. It will be discussed in the context of the whole sub-package structure for Gammapy in separate PIG, before Gammapy v1.0


from gammapy.models import Powerlaw, SpatialGaussian, SourceModel

# or consistently with astropy

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

Astropy has the astropy.modeling.models. You claim here or make it sound like they are in astropy.modeling.

This comment has been minimized.

@adonath

adonath Jan 14, 2019
Author Member

See above, section removed.

from gammapy.modeling import Powerlaw, SpatialGaussian, SourceModel


List of Pull Requests

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

Suggest to number the pull requests, i.e. make a bullet list like this:

#. one
#. two

There's 10+, so discussion is easier if there's numbers, and I also think it's good to see how many there are.

Also, I think several should be split in two PRs. In my experience it's more efficient to have small PRs.

I also think it would be helpful to suggest a development cycle (v0.10, v0.11, v0.12) where this could go.
I.e. basically this PIG very clearly outlines the work to be done, so that people can just pick up a task.
Only if unclear / needed could the lead devs then make an issue for each task that describes it in greater detail, otherwise someone would just start a PR (can be with just a few issues) and discussion could go there directly. This would guarantee that discussion is concrete and useful, and avoid very much time spent on planning and disucssion ahead, without it being clear if / who will do the work.

I would suggest to put the three PRs with "rename" at the end. They are very disturbing / break every user script. And really aren't high priority, not needed to make progress. They can also be in the middle like you have it now, but then they should probably be all in one given release (e.g. v0.11), instead of being spread across many releases, no?

=====================

This is a proposal for a list of pull requests implementing the proposed changes,
order by priority:

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

order -> ordered

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

Fixed

- Implement support for model component names in `SkyModel`. Implement `SkyModels.__getitem__`
to allow for access of model components by name.

- Fix existing XML I/O of `SkyModels` and add serialization of the model component

This comment has been minimized.

@cdeil

cdeil Dec 20, 2018
Member

The model serialisation is a big and pretty much independent task. Maybe put it at the end?
E.g. currently you have evaluation_radius or sky coord frame below model serialisation in terms of priority.
Shouldn't those be higher priority?

---------------------------------
For any kind of IACTs analysis hadronic background models are required, which model
the residual hadronic (gamma-like) background emission from templates. They differ
from source templates, because they already have the IRFs applied. We propose

This comment has been minimized.

@registerrier

registerrier Dec 20, 2018
Contributor

I would write 'because they are defined in reconstructed spatial and energy coordinates'

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

Done

objects.


Introduction of background models

This comment has been minimized.

@registerrier

registerrier Dec 20, 2018
Contributor

I don't think the dichotomy BackgroundModel-BackgroundIRFModel is really required. A BackgroundModel is always in reco space and no IRF is to be applied.

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

Agreed

from gammapy.spectrum import Powerlaw, SpectralIRFModel

pwl = Powerlaw()
spectral_irf_model = SpectralIRFModel(model=pwl, aeff=aeff, livetime=livetime,

This comment has been minimized.

@registerrier

registerrier Dec 20, 2018
Contributor

A priori this can be done. But this breaks the logic of the current spectral analysis implementation, where livetime is a member of the PHACountSpectrum.

of this hierarchical model component structure is intrinsically difficult. For
this reason we propose to first support and improve the existing `SourceModels` that
implements an easier to handle flat hierarchy for model components. Support for
arbitrary model arithmetics can be introduced, if needed, after Gammapy v1.0. We

This comment has been minimized.

@registerrier

registerrier Dec 20, 2018
Contributor

I think one the use cases that are not covered by a list of models are the multiplicative models. The most common example would be the absorbed spectrum. At the moment we use a trick with the AbsorbedSpectrumModel rather than multiplicative model. This is probably sufficient for a large number of use cases.

spectral_irf_model = SpectralIRFModel(model=pwl, aeff=aeff, livetime=livetime,
edisp=edisp)
npred = spectral_irf_model.npred
dnde = spectral_irf_model.dnde

This comment has been minimized.

@registerrier

registerrier Dec 20, 2018
Contributor

I am not sure .dndeand .flux are really relevant here. Those are intrinsic unconvolved quantities, they belong to SpectralModel. The only relevant method here is .npred

|BackgroundModel |BackgroundTemplate? |BackgroundIRFModel |Arithmetic background |
|(None) |(None) | (None) |models? |
+-----------------+----------------------------------------------+----------------------+-----------------------------+

This comment has been minimized.

@registerrier

registerrier Dec 20, 2018
Contributor

I wonder if the scheme here is not a bit too systematic. SourceIRFModel and BackgroundIRFModel are the same thing in a sense. They contain a MapGeom and have a .npred method.

Isn't the relevant point here, the geometry?

A convolved model is meant to return the npred on a given geometry (1D, 2D or 3D). Therefore we could rather have a MapModel that can be either an IRFMapModel or a TemplateMapModel.

Doing so might also alleviate the issue of SpatialIRFModel since a Map can be 2D or 3D.

This comment has been minimized.

@adonath

adonath Jan 3, 2019
Author Member

Just to clarify: SourceIRFModel and BackgroundModel are not the same thing. Yes, both return the predicted number of counts, but the purpose of the SourceIRFModel is to bundle the (differential) model component with it's corresponding IRFs., while the BackgroundModel represents an already integrated model, which just introduces additional scaling parameters for norm and tilt. I agree the BackgroundModel could be implemented using a more general MapModel object...I'll re-think this part.

@cdeil
Copy link
Member

@cdeil cdeil commented Dec 21, 2018

@adonath - I started a notebook today to look a bit how linked parameters work in other modeling frameworks. It's at https://github.com/gammapy/gammapy-extra/blob/master/experiments/parameter_links.ipynb .

I didn't get very far, and don't have time in the coming weeks, so feel free to continue there or reference it from the PIG if you think it's useful.

The main question IMO is how to set up models and parameters for multiple datasets and concrete changes and additions needed for modeling in v0.10 or v0.11. If you think that can be done by referring to the same Parameter objects from many models and only minimal changes to the modeling framework, great!

I'm not very happy with the fact that we can't pass existing Parameter to Model.__init__, or that we need to access the model.parameters.parameters list to create links. IMO it should be model.parameters._parameters, i.e. an implementation detail, and a minimal API should be offered on the Model or Parameters class to do what's needed, e.g. to create links.

But given that (at least for me) there's no clear "best" modeling API / framework, I think anything that gets the main use cases done and we should get datasets in early 2019, and then we can always refactor and improve the modeling framework later in the year. So even just using the existing post-init access internals way would be fine with me, if it allows getting datasets e.g. for v0.11.

a = ConstantModel(1)
b = ConstantModel(2)

# Create link
a.parameters.parameters[0] = b.parameters['const']
@adonath adonath force-pushed the adonath:models_pig branch 8 times, most recently from eccdf62 to cbe91c3 Jan 14, 2019
@adonath adonath force-pushed the adonath:models_pig branch from cbe91c3 to 81d6cfc Feb 11, 2019
@adonath adonath modified the milestones: 0.11, 0.12 Mar 8, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented May 3, 2019

I know that we've discussed this before, but I can't find a mention of parameter linking and distributed or parallel computing above!?

Anyways, my main points are that we should try to develop modeling & fitting in Gammapy to support

  1. distributed parallel computing
  2. gradients

It doesn't have to be in Gammapy v1.0 in fall, but it should be there in the coming years.
There are things we can already do now to support developments in this direction.

Motivation for parallel computing: currently Gammapy runs in a single CPU process, but most users have access to multi-core CPUs or CPU clusters, that if used properly would give 10x to 1000x speedups. This is often "nice to have" to run an analysis in 1 minute instead of 10 minutes or 1 hour, but there are use cases where great speedups are required - e.g. Galactic center analysis with 10 overlapping sources and 100s of observations.

Motivation for gradients: less strong that need for parallel computing, for fitting up to ~ 10 sources we could probably get all results with our current optimisers (which often do numeric gradients by stepping in each direction), but overall gradient-based optimisers are often more reliable (more accurate gradients) and efficient, especially with many parameters (say ~ 100 parameters from the ~10 sources).

Suggestions:

  • Replace the current system of linked parameters across datasets via Python objects in Python lists attached to the datasets with a new system where parameters are updated by passing new values, and the linking is handled in some other way. You can see e.g. in https://github.com/ray-project/ray how the core of a distributed system for optimisation using an update function. If this exists, prototyping with distributed computing can start.

  • Consider avoiding the use of Astropy objects (Quantity & SkyCoord) during model evaluation. If only Numpy is used, then experimenting with alternative systems that support gradients and more efficient computation (like https://github.com/HIPS/autograd or dask / pytorch / ...) becomes possible.

@adonath - what do you think? what is feasible for v1.0 (fall 2019), and what should be listed as possible directions for later (e.g. v2.0 in 2020 or even later)?

@adonath adonath modified the milestones: 0.12, 0.13 May 28, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented Jun 24, 2019

Since this PIG currently doesn't mention "uncertainty" or "covariance" or "samples" or "MCMC" or "Bayesian", I've split out that part into it's own PIG: #2255
But both PIGs are basically about Gammapy modeling, we should make them a priority and try to finish them in July.

@cdeil cdeil modified the milestones: 0.13, 0.14 Jul 18, 2019
@adonath adonath modified the milestones: 0.14, 0.15 Sep 26, 2019
adonath added 2 commits Dec 17, 2018
@adonath adonath force-pushed the adonath:models_pig branch from 81d6cfc to a087fde Oct 7, 2019
@adonath
Copy link
Member Author

@adonath adonath commented Oct 18, 2019

In a087fde, I have withdrawn the PIG. Will merge the PR now...

@adonath adonath merged commit f4df5ec into gammapy:master Oct 18, 2019
7 of 9 checks passed
7 of 9 checks passed
gammapy.gammapy Build #20191007.1 had test failures
Details
gammapy.gammapy (Test Python36) Test Python36 failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
gammapy.modeling automation moved this from In progress to Done Oct 18, 2019
@cdeil
Copy link
Member

@cdeil cdeil commented Oct 18, 2019

@adonath - Thanks for closing this out, and all your work on modeling in the last year.

Please add a link to this PIG from docs/development/pigs/index.rst, so that it shows up in the docs.

I think we should still put a closing date. Maybe like this?

Withdrawn: Oct 18, 2019

instead of the empty

Accepted:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants