Enforcing SACC file/object as an input to build_likelihood method

[arthurmloureiro]

Hi!

So from building the blinding infrastructure in a way that is "likelihood agnostic", I noticed one potential issue which I cannot protect the user from and that is having a hardcoded path to a sacc file in the likelihood.

For example, in the new firecrown tutorial documents we have an example where the SACC file is somewhat harcoded to the build_likelihood() function:

SACC_FILENAME = os.path.expandvars( \
    "${FIRECROWN_DIR}/examples/srd_sn/srd-y1-converted.sacc" \
    )

def build_likelihood(_):
    snia_stats = sn.Supernova(sacc_tracer="sn_ddf_sample")
    likelihood = ConstGaussian(statistics=[snia_stats])
    sacc_data = sacc.Sacc.load_fits(SACC_FILENAME)
    likelihood.read(sacc_data)
    # This simple likelihood has no associated ModelingTools
    return likelihood, None

Meanwhile, TXPipe deals with this in a different way:

def build_likelihood(build_parameters):
    """
    This is a generic 3x2pt theory model, but with only
    bias as a systematic.

    """
    sacc_data = build_parameters['sacc_data']
[...]

For the blinding infrastructure, since we want the user to be able to blind a sacc file from the command line (as well as to import the library and use it in TXPipe) there is a potential issue if the user provides a likelihood for the blinding that works in the way that the first example above works because we use the likelihood method get_data_vector() to obtain the DV to be blinded. If we have a def build_likelihood(_) with a hardcoded sacc file, the blinding will pass the user sacc file to this likelihood module which is ignored, using the hardcoded sacc instead.

I don't seem to have a way to impose that any likelihoods passed to the blinding infrastructure should take a sacc file as input to the build_likelihood function.

In my likelihood example for the blinding, I even have to implement some hacking to protect from giving the SACC file in different ways:

def build_likelihood(build_parameters):
    """
    Create a firecrown likelihood for a cosmic shear analysis.
    
    Parameters
    ----------
    build_parameters : dict
        Dictionary of parameters to be used in the likelihood.

    Returns
    -------
    likelihood : firecrown.likelihood.likelihood.Likelihood
        A firecrown likelihood object.
    """

    try:
        sacc_data = build_parameters['sacc_data']
    except TypeError:
        sacc_data = build_parameters.data['sacc_data']

    # This is a trick TXPipe uses to pass a sacc file or object
    # to the likelihood. We need to check if the input is a string
    if isinstance(sacc_data, (str, pathlib.Path)):
        sacc_data = sacc.Sacc.load_fits(sacc_data)
    else:
        sacc_data = sacc_data.copy()

Personally, I think that we should discourage the user from creating likelihoods with hardcoded data-vectors. Is that something others agree? that maybe we should impose that the build_likelihood method must receive an input which contains either a sacc object or a path to a sacc file?

Cheers!

[arthurmloureiro]

Tagging @marcpaterno @vitenti and @tilmantroester here for the discussion 😄

[tilmantroester]

Related: #394
Maybe changing the example to something that's not using a hardcoded sacc file might be a partial but quick fix.

[arthurmloureiro]

Agreed! We had people in the Forecasting Workshop struggling to understand how not to give a hardcoded sacc to the cosmosis firecrown likelihood.

[arthurmloureiro]

I really think we should enforce that sacc files cannot be hardcoded into the likelihood.

[marcpaterno]

The factory mechanism introduced into Firecrown v1.8 allows the (name of the) SACC file to be passed as a parameter.

[tilmantroester]

There can be legitimate uses for hardcoding a sacc file in a likelihood, e.g. in cases where the likelihood is specific to this sacc file.

[arthurmloureiro]

There can be legitimate uses for hardcoding a sacc file in a likelihood, e.g. in cases where the likelihood is specific to this sacc file.

Hi @tilmantroester I think in codes like cosmosis, for example, even in these cases we have the config pointing to the data.

These cases should be avoided, in my fair opinion. There's no way to ensure this is not happening for the blinding and that's a bigger problem than allowing the user to have bad habits.

[tilmantroester]

I agree that having the hardcoded paths in the examples is not ideal but I don't think it's technically feasible to enforce the absence of hardcoded paths. The best and only option is to have the blinding code check that the file that was loaded into the likelihood is indeed the one that it's supposed to.

[arthurmloureiro]

You can enforce that that a sacc path needs to be an input to the build_likelihood function.
I think this pretty much stops the sacc being hardcoded?

The best and only option is to have the blinding code check that the file that was loaded into the likelihood is indeed the one that it's supposed to.
I can try that! I think I tried it back in the days and failed, but I can try again ! Thanks for the idea 😄

[tilmantroester]

I think this pretty much stops the sacc being hardcoded?

Someone could still load a different sacc file within the build_likelihood function, either intentionally or by accident. I don't see how you could prevent that.

I can try that! I think I tried it back in the days and failed, but I can try again ! Thanks for the idea 😄

I'm not sure that functionality exists yet but @marcpaterno 's proposal above would address this.