Parameters and names

[klieret]

Hi Simon,

it seems like that if you add 2 pdfs of the same type, the parameters will overwrite each other, even if you assign them different names:

MWE (updated):

from pyroofit.models import Gauss
pdf1 = Gauss(('x', -3, 3), mean=(-1, 0, 1), name="first_gauss")
pdf2 = Gauss(('x', -3, 3), mean=( -1, 0, 1), name="second_gauss")
pdfsum = pdf1 + pdf2
print(pdfsum.parameters)

returns

{'n_first_gauss': <ROOT.RooRealVar object ("n_first_gauss") at 0x4985d40>, 'mean': <ROOT.RooRealVar object ("second_gauss_mean") at 0x4947690>, 'sigma': <ROOT.RooRealVar object ("second_gauss_sigma") at 0x4933690>, 'n_second_gauss': <ROOT.RooRealVar object ("n_second_gauss") at 0x49b7750>}

I think I looked in the code a while ago and you were checking only for the dictionary keys, not for the names of the RooFit objects and therefore overwrite the mean parameter.

Perhaps I'll create a PR for this later if I find the time, just wanted to report this, because it seems to be important to fix this fast.

Cheers,
Kilian

[klieret]

I see that problem in pdf.add_parameter:

When specifying a parameter with name mean=("mean1", -1, 0, 1)), we call create_roo_variable(("mean1", -1, 0, 1), "some default name"), which then creates a RooRealVar called mean1 (which is what we want).

In pdf.add_parameter, we ignore the possibility for specifying the name:

def add_parameter(self, param_var, param_name=None, final_name=None, **kwds):
        """ Add fit parameter

        Args:
            param_var (list or ROOT.RooRealVar): Initialisation of the parameter as list or ROOT object
            param_name (:obj:`str`): Name of the parameter within the object (Not within ROOT namespace!)
            final_name (:obj:`str`, optional): Name if the parameter within PDF and ROOT namespace
            **kwds: create_roo_variable keywords

        Returns:
            ROOT.RooRealVar reference to fit parameter

        """
        if final_name is None:
            assert param_name is not None, "Please specify a parameter name"
            name = self.name + '_' + param_name
        else:
            name = final_name
        roo_param = create_roo_variable(param_var, name=name, **kwds)
        self.parameters[param_name] = roo_param
        self.parameter_names[param_name] = name
        self.__setattr__(param_name, roo_param)
        return self.parameters[param_name]

By changing that as follows:

def add_parameter(self, param_var, param_name=None, final_name=None, **kwds):
        """ Add fit parameter

        Args:
            param_var (list or ROOT.RooRealVar): Initialisation of the parameter as list or ROOT object
            param_name (:obj:`str`): Name of the parameter within the object (Not within ROOT namespace!)
            final_name (:obj:`str`, optional): Name if the parameter within PDF and ROOT namespace
            **kwds: create_roo_variable keywords

        Returns:
            ROOT.RooRealVar reference to fit parameter

        """
        if final_name is None:
            assert param_name is not None, "Please specify a parameter name"
            name = self.name + '_' + param_name
        else:
            name = final_name
        roo_param = create_roo_variable(param_var, name=name, **kwds)
        self.parameters[name] = roo_param
        self.parameter_names[name] = roo_param.GetName()
        self.__setattr__(name, roo_param)
        return self.parameters[name]

everything works fine again.

There's also a second fix in there: I suppose that self.parameter_names is supposed to hold the RooRealVar internal name, right? But if we specify a name in the tuple, e.g. ("mean1", -1, 0, 1), this was not the case any longer.

In general, the whole naming thing seems to be a bit confusing to me and is not yet much documented (especially when giving the name in the tuple, it's not very clear what takes precedence).

I also don't quite understand why don't enforce the name of the parameter to always be the same of the RooRealVar internal name (or vice-versa), I don't see any use case there and it was very confusing to me at first.

[simonUU]

Hi Kilian, sorry for the late reply, this is a bug in AddPdf, so in this case you would need to extract the parameters from the constituents.
I am working on a fix for this!

[klieret]

Yes, sorry I also saw in the meantime that it works as far as the fitting is concerned and that just pdf.parameters gives confusing output.

If you don't have the time I could also take another look at this in the next weeks.

[simonUU]

One of the main reasons I do not want to use your suggestion is, that I would like a Gauss to always have a 'mean' not a 'gauss1_mean' or so... for saving parameters and so on.
But there should be an easy fix at least for the AddPdf naming of the parameters.

[klieret]

Hmm, but I only see 3 options (but might overlook something):

1. Remove .parameters and get_parameters from AddPdf because they don't work due to key name clashes. Users could still use for pdf in add_pdf: pdf.get_parameters() to get the parameters. But that isn't really satisfying.
2. Let .get_parameters return a double dict with the pdf name at first level. But as this method is inherited from the pdf base class, it should return the same format everywhere.
3. Use different parameter names in different objects.

I also think that keeping keys/names that are different from the names of the ROOT objects (as in x.GetName() will almost always lead to confusion. I think the clearest and cleanest way is to just always exactly use the name as given by the user for everything.

[simonUU]

#5 might do the trick.

I also think that keeping keys/names that are different from the names of the ROOT objects (as in x.GetName() will almost always lead to confusion. I think the clearest and cleanest way is to just always exactly use the name as given by the user for everything.

I see the point, but imagine you want to save parameters to a file, there you would describe a Gauss of course with a 'mean' and 'sigma' not some other names..