BUG: the sherpa.astro.optical.LogEmission model creates an unused parameter

[DougBurke]

The sherpa.astro.optical module creates a number of models which use "hidden" parameters (which aren't really used elsewhere). This includes the LogEmission model, which creates an index parameter but then doesn't use it.

In the current master branch, the index parameter is created at

sherpa/sherpa/astro/optical/__init__.py

Line 566 in a679e7f

self.limit = Parameter(name, 'limit', 4., alwaysfrozen=True,

but you can see that it is not used in the calc method - i.e. no mention of p[4] or self.index in

sherpa/sherpa/astro/optical/__init__.py

Line 574 in a679e7f

def calc(self, p, x, xhi=None, **kwargs):

I think in this case we can just remove this parameter from the model since users will have had to dig around to find it in the first place (e.g. in the .pars attribute of the model instance) as it is not visible with a print call:

>>> mdl = optical.LogEmission()
>>> print(mdl)
logemission
   Param        Type          Value          Min          Max      Units
   -----        ----          -----          ---          ---      -----
   logemission.fwhm thawed          100  1.17549e-38  3.40282e+38       km/s
   logemission.pos frozen         5000  1.17549e-38  3.40282e+38  angstroms
   logemission.flux thawed            1 -3.40282e+38  3.40282e+38           
   logemission.skew frozen            1  1.17549e-38  3.40282e+38           

>>> mdl.pars 
(<Parameter 'fwhm' of model 'logemission'>,
 <Parameter 'pos' of model 'logemission'>,
 <Parameter 'flux' of model 'logemission'>,
 <Parameter 'skew' of model 'logemission'>,
 <Parameter 'limit' of model 'logemission'>)

The alternative (which is what I would suggest doing if it were more user-visible) would be to add a deprecation warning the first time that the parameter is accessed (assuming we can do this; it might require some work to add this functionality to the model class in the first place). This deprecation would be in place for the 4.9.x series, and then the attribute removed after this.

[DougBurke]

@jbudynk would the removal of the limit parameter be an issue for ChandraCXC/iris? I don't know how it uses these models.

[dtnguyen2]

One would think that with all the discussions on this issue, it would be clear what has to be done here and yet I'm still confused. Go figure! The parameter limit is defined with the option: hidden=True. If the option hidden=True is removed:

(sherpa) [dtn@devel12 sherpa]$ git diff sherpa/astro/optical/__init__.py
diff --git a/sherpa/astro/optical/__init__.py b/sherpa/astro/optical/__init__.py
index 353ce809..ab261413 100644
--- a/sherpa/astro/optical/__init__.py
+++ b/sherpa/astro/optical/__init__.py
@@ -1164,8 +1164,7 @@ class LogEmission(RegriddableModel1D):
                              units='angstroms')
         self.flux = Parameter(name, 'flux', 1.)
         self.skew = Parameter(name, 'skew', 1., tinyval, frozen=True)
-        self.limit = Parameter(name, 'limit', 4., alwaysfrozen=True,
-                               hidden=True )
+        self.limit = Parameter(name, 'limit', 4., alwaysfrozen=True)
 
         ArithmeticModel.__init__(self, name,
                                  (self.fwhm, self.pos, self.flux,

Then the limit parameter limit will be printed:

>>> print(mdl)
logemission
   Param        Type          Value          Min          Max      Units
   -----        ----          -----          ---          ---      -----
   logemission.fwhm thawed          100  1.17549e-38  3.40282e+38       km/s
   logemission.pos frozen         5000  1.17549e-38  3.40282e+38  angstroms
   logemission.flux thawed            1 -3.40282e+38  3.40282e+38           
   logemission.skew frozen            1  1.17549e-38  3.40282e+38           
   logemission.limit frozen            4 -3.40282e+38  3.40282e+38           

The other option is to fix so that the limit parameter does not get printed when the following command is executed (which I think is the original intention of the hidden=True option):

>>> mdl.pars

[dtnguyen2]

@DougBurke is right, the sherpa.astro.optical.LogEmission only as four parameters (fwhm, pos, flux and skew) so it is a no-brainer to eliminate the fifth entry (limit) in the parameter definition.

    def __init__(self, name='logemission'):

        self.fwhm = Parameter(name, 'fwhm', 100., tinyval, hard_min=tinyval,
                              units="km/s")
        self.pos = Parameter(name, 'pos', 5000., tinyval, frozen=True,
                             units='angstroms')
        self.flux = Parameter(name, 'flux', 1.)
        self.skew = Parameter(name, 'skew', 1., tinyval, frozen=True)

        ArithmeticModel.__init__(self, name,
                                 (self.fwhm, self.pos, self.flux,
                                  self.skew, self.limit))

    # @modelCacher1d
    def calc(self, p, x, xhi=None, **kwargs):
        x = numpy.asarray(x, dtype=SherpaFloat)

        if 0.0 == p[0]:
            raise ValueError('model evaluation failed, ' +
                             '%s fwhm cannot be zero' % self.name)

        if 0.0 == p[1]:
            raise ValueError('model evaluation failed, ' +
                             '%s pos cannot be zero' % self.name)

        if 0.0 == p[3]:
            raise ValueError('model evaluation failed, ' +
                             '%s skew cannot be zero' % self.name)

        arg = 0.69314718 / numpy.log(1.0 + p[0] / 2.9979e5 / 2.0)
        if (arg <= 1.0):
            arg = 1.0001

        fmax = (arg - 1.0) * p[2] / p[1] / 2.0

        if (p[3] == 1.0):
            return numpy.where(x >= p[1],
                               fmax * numpy.power((x / p[1]), -arg),
                               fmax * numpy.power((x / p[1]), arg))

        arg1 = 0.69314718 / numpy.log(1.0 + p[3] * p[0] / 2.9979e5 / 2.0)
        fmax = (arg - 1.0) * p[2] / p[1] / (1.0 + (arg - 1.0) / (arg1 - 1.0))

        return numpy.where(x <= p[1],
                           fmax * numpy.power((x / p[1]), arg),
                           fmax * numpy.power((x / p[1]), -arg1))