Action #773

Feature #692: Perform an extensive interface review of all classes

Review model module classes

Added by Knödlseder Jürgen almost 12 years ago. Updated about 11 years ago.

Status:ClosedStart date:02/20/2013
Priority:NormalDue date:
Assigned To:Knödlseder Jürgen% Done:

100%

Category:-Estimated time:20.00 hours
Target version:00-08-00
Duration:

Recurrence

No recurrence.

Related issues

Related to GammaLib - Action #693: Review interface for GModelSpectral classes Closed 01/12/2013

History

#1 Updated by Knödlseder Jürgen almost 12 years ago

The following classes have been reviewed:
Class Comment
GModels Derive from GContainer, implement container methods.
GModelPar Revise interface completely, add autoscale() method, assure non-zero scale factor.
GModel Return name by const reference
GModelSky value and gradients take GPhoton as argument, inline declarations, improved documentation
GModelSpatial Add autoscale() method, pass GPhoton to eval and eval_gradients methods, and GEnergy and GTime to mc method, inline declarations
GModelSpatialDiffuse Pass GPhoton to eval and eval_gradients methods, and GEnergy and GTime to mc method
GModelSpatialDiffuseConst Pass GPhoton to eval and eval_gradients methods, and GEnergy and GTime to mc method, inline declarations
GModelSpatialDiffuseCube Pass GPhoton to eval and eval_gradients methods, and GEnergy and GTime to mc method, inline declarations
GModelSpatialDiffuseMap Pass GPhoton to eval and eval_gradients methods, and GEnergy and GTime to mc method, inline declarations
GModelSpatialElliptical Pass GEnergy and GTime to eval, eval_gradients and mc methods, inline declarations
GModelSpatialEllipticalDisk Pass GEnergy and GTime to eval, eval_gradients and mc methods, inline declarations
GModelSpatialPointSource Pass GPhoton to eval and eval_gradients methods, and GEnergy and GTime to mc method
GModelSpatialRadial Pass GEnergy and GTime to eval, eval_gradients and mc methods, inline declarations
GModelSpatialRadialDisk Pass GEnergy and GTime to eval, eval_gradients and mc methods, inline declarations
GModelSpatialRadialGauss Pass GEnergy and GTime to eval, eval_gradients and mc methods, inline declarations
GModelSpatialRadialShell Pass GEnergy and GTime to eval, eval_gradients and mc methods, inline declarations
GModelSpectral Add autoscale() method, add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients
GModelSpectralConst Add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, add value constructor and access methods, rename norm to value
GModelSpectralExpPlaw Remove autoscale() method, auto-scale in parameter constructor, complete value constructor, add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, implement parameter access methods (inline), add pre computation cache for eval and eval_gradients methods, rename norm to prefactor and ecut to cutoff
GModelSpectralFunc Complete value constructor, add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, implement parameter access methods (inline)
GModelSpectralPlaw Remove autoscale() method, auto-scale in parameter constructor, complete value constructor, add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, implement parameter access methods (inline), rename norm to prefactor
GModelSpectralPlaw2 Complete value constructor, add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, implement parameter access methods (inline), emin() and emax() methods now return / take GEnergy
GModelSpectralNodes Add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, implement parameter access methods and node manipulation methods
GModelSpectralLogParabola Remove autoscale() method, auto-scale in parameter constructor, complete value constructor, add time argument to eval, eval_gradients and mc methods, remove const from eval_gradients, implement parameter access methods (inline), pivot() methods now return / take GEnergy, rename norm to prefactor
GModelTemporal Remove const from eval_gradients
GModelTemporalConst Add value contructor, remove const from eval_gradients, implement parameter access methods (inline)
GModelData Nothing
GModelRegistry size method inline
GModelSpatialRegistry size method inline
GModelSpectralRegistry size method inline
GModelTemporalRegistry size method inline

#2 Updated by Knödlseder Jürgen almost 12 years ago

  • Status changed from New to In Progress
  • Assigned To set to Knödlseder Jürgen
  • % Done changed from 0 to 10

#3 Updated by Knödlseder Jürgen almost 12 years ago

  • Remaining (hours) changed from 20.0 to 16.0

#4 Updated by Knödlseder Jürgen almost 12 years ago

  • Remaining (hours) changed from 16.0 to 15.0

The GModels class access operator now returns models by reference instead of returning pointers.

#5 Updated by Knödlseder Jürgen almost 12 years ago

Jürgen Knödlseder wrote:

The GModels class access operator now returns models by reference instead of returning pointers.

I just recognized that this was stupid. I reintroduced the code slicing problem. Instead, all container classes holding pointers of base classes should have pointer access operators! See #517.

#6 Updated by Knödlseder Jürgen almost 12 years ago

The GModels method should have a hasmodel() method that checks whether a model with a specific name is present in the container. The method should be inspired from the GPars::haspar() method.

In general, container classes that allow access by name should have a hasXXX method to check whether a specific named object is present.

#7 Updated by Knödlseder Jürgen almost 12 years ago

A generic autoscale() method should be added to a model components. The purpose of the autoscale() method is to set the scale factors to the real value and the value to 1, so that all model parameters are scaled using their actual values.

The best is to add an autoscale() method at the GModelPar level, so that each parameter can be autoscaled.

#8 Updated by Knödlseder Jürgen almost 12 years ago

  • % Done changed from 10 to 20

Jürgen Knödlseder wrote:

A generic autoscale() method should be added to a model components. The purpose of the autoscale() method is to set the scale factors to the real value and the value to 1, so that all model parameters are scaled using their actual values.

The best is to add an autoscale() method at the GModelPar level, so that each parameter can be autoscaled.

An autoscale() method has been added to GModelPar and to the GModelSpatial, GModelSpectral and GModelTemporal classes. This method should be called systematically in the parameter constructors.

#9 Updated by Knödlseder Jürgen almost 12 years ago

  • % Done changed from 20 to 50
  • Remaining (hours) changed from 15.0 to 10.0

The GModelPar interface has been fully revised. This impacted many of the model classes.

The modification has been tested, and ctools compliance checked. Now the road is paved towards a review of all other interfaces.

#10 Updated by Knödlseder Jürgen almost 12 years ago

I just recognized a major problem with the GModelSpatialDiffuseCube class: this class does not only represent a spatial model, but a spatio-spectral model. As GModelSpatialDiffuseCube has the interface of a spatial model only, it does not allow energy dependent model evaluation. In fact, GModelSpatialDiffuseCube is not a component of a fully factorized model, it is the first example of a non-factorized model.

How to deal with that? Change the class inheritance scheme where GModelSky is only the base class and then we have derived classes for factorized or other model types? Or making GModelSky deal with all model types?

Or should we treat GModelSpatialDiffuseCube as a special case, because it is a spatio-spectral model but coexists with a spectral model in the Fermi-LAT XML interface?

The problem occurs in the response computation, which explicitly uses the spatial model’s eval method with the sky direction as single parameter for model computation. The source energy is available at this point, so it could be used for computation.

It is interesting to recognized that the GModelSky::spatial method does not really relies on the fact that the model is factorized, except for the fact that the gradients need the correct pre-factors. This means that the irf method could in fact return the full response, and not only that of the spatial component.

The entire interfaces were in fact thought for a factorized sky model, and now we have a clear example for a non-factorized sky model.

A created action #808 to rethink the interface for non-factorized sky models.

#11 Updated by Knödlseder Jürgen almost 12 years ago

  • % Done changed from 50 to 70

Finished the spatial classes.

#12 Updated by Knödlseder Jürgen over 11 years ago

Jürgen Knödlseder wrote:

The GModels method should have a hasmodel() method that checks whether a model with a specific name is present in the container. The method should be inspired from the GPars::haspar() method.

In general, container classes that allow access by name should have a hasXXX method to check whether a specific named object is present.

Done!

#13 Updated by Knödlseder Jürgen over 11 years ago

  • Status changed from In Progress to Feedback
  • % Done changed from 70 to 100
  • Remaining (hours) changed from 10.0 to 0.0

The model class review is now finished.

#14 Updated by Knödlseder Jürgen about 11 years ago

  • Status changed from Feedback to Closed

Also available in: Atom PDF