Allow default parameter values

[tilmantroester]

This adds optional default parameters to ParamsMap.get_from_prefix_param. This is motivated by issue #193, which could have been avoided by having default values in the code, rather than some ini file. Before merging, the correct value for alphag needs to be confirmed though.

[marcpaterno]

Cobaya requires that the names of all parameters be specified. We rely on the RequiredParameters object containing a list of names to let us know what to tell Cobaya is required. The introduction of default into this system would break this; we would not know whether to tell Cobaya that alphag is required or not.

In addition, it looks to us like this design is vulnerable to an easy-to-make user error. If a user misspells a parameter name (e.g. writes alpha_g rather than alphag, the extraneous name alpha_g will be silently ignored, and the default value for alphag will be used.

Please take a look at how the optional mag_bias is handed in NumberCounts. We are using name-only arguments in constructors, sometimes with default values, to help make sure that an accidental misspelling of a parameter name in a configuration will not pass quietly, and lead to unwanted behavior. Can a similar design meet your needs here?

[tilmantroester]

In addition, it looks to us like this design is vulnerable to an easy-to-make user error. If a user misspells a parameter name (e.g. writes alpha_g rather than alphag, the extraneous name alpha_g will be silently ignored, and the default value for alphag will be used.

For cosmosis there is a flag that raises an error when a parameter is never accessed IIRC.

I'd argue that having to include parameters in a configurations file that the user doesn't need or understand but are necessary for a general form of a model is more prone to user error. For an explicit example, see issue #193.

If cobaya doesn't support the concept of optional parameters then the implementation might need to change how the required parameters are set up. But we need some way to implement systematics where a subset of the parameters have default values that can however be varied for specific analyses.

Here's an implementation that works with cobaya but is a bit verbose for my taste. Setting ia_a_d=None makes it a required parameter again. But I haven't checked what happens in the case of per-source parameters. It would be nice if populating the self.params_with_defaults list were automatic.

def __init__(self, sacc_tracer: Optional[str] = None, ia_a_d=0.7):
        super().__init__()
        self.sacc_tracer = sacc_tracer
        self.params_with_defaults = []
        if ia_a_d is not None:
            self.params_with_defaults += ["ia_a_d"]
            self.a_d = ia_a_d

    @final
    def _update(self, params: ParamsMap):
        self.a_1 = params.get_from_prefix_param(self.sacc_tracer, "ia_a_1")
        self.a_2 = params.get_from_prefix_param(self.sacc_tracer, "ia_a_2")
        if "ia_a_d" not in self.params_with_defaults:
            self.a_d = params.get_from_prefix_param(self.sacc_tracer, "ia_a_d")

    @final
    def required_parameters(self) -> RequiredParameters:
        return RequiredParameters(
            [parameter_get_full_name(self.sacc_tracer, pn) for pn in self.params_names if pn not in self.params_with_defaults]
        )

[marcpaterno]

We have tried to make a general solution. We have provided an example below. In this design:

1. For clarity: a "required parameter" means that parameter is required to be read from the sample (e.g. the CosmoSIS datablock).
2. Some parameters are always required; in the code below ia_a_1 and ia_a_2 are of this nature.
3. Other parameters are required or not, under the user's control (the user is the author of the likelihood factory function which calls the constructor of the Updatable in question). It is the value passed to the constructor that is used to make the decision. The value may be there for either of two reasons: (1) because the user specified it or (2) because the user did not specify it but the author of the Updatable specified a default.
4. The author of the Updatable class can provide default values, either None or some float value, or not provide a default, as seems most fitting.
5. The value actually used by the constructor at runtime determines whether the parameter in question is required or not:
   1. if the value is None the parameter is required; it must be provided by the sampler, and a runtime error will happen if it is not.
   2. if the value is any float value the parameter is not required; it must not be provided by the sample, and a runtime error will happen if it is provided.

Here is our proposed replacement for your code above:

def __init__(self, sacc_tracer: Optional[str] = None, *, ia_a_d=0.7, ia_x=None, ia_y):
    super().__init__()
    self.sacc_tracer = sacc_tracer
    # We hope to make the next line more automated...
    self.required_parameter_names = ["ia_a_1", "ia_a_2"] # always required

    # We hope to be able to automate the following through introspection    
    if ia_a_d is None:
        self.required_parameter_names += ["ia_a_d"]
    if ia_x is None:
        self.required_parameter_names += ["ia_x"]
    if ia_y is None:
        self.required_parameter_names += ["ia_y"]

@final
def _update(self, params: ParamsMap):
    # We may be able to push this implementation into the framework
    for name in required_parameter_names:
        setattr(self, name, params.get_from_prefix_param(self.sacc_tracer, name))

@final
def required_parameters(self) -> RequiredParameters:
   # We may be able to push this implementation into the framework
    return RequiredParameters([parameter_get_full_name(self.sacc_tracer, pn) 
                               for pn
                   in self.required_parameter_names])
 )

Does this meet your needs?

[tilmantroester]

Looks good to me!

[vitenti]

@marcpaterno @tilmantroester , this PR ready to review. @am610 Ayan, please note that I had to include the sacc tracer into the supernova parameter M, the default behavior to a Updatable parameter name is to include the sacc tracer as its prefix to avoid name clashing between different observables/likelihoods. Now this must be called sn_ddf_sample_m (or sn_ddf_sample_M) instead of simply M.