PR 395 broke support for bandpower window functions

[tilmantroester]

The refactoring of TwoPoint in #395 broke the support for bandpower window functions (e.g. https://github.com/LSSTDESC/forecasting/blob/main/forecast_inputs/datafiles/sacc_files/data_and_cov/forecast_fid_3x2pt_linear_sys_20_limber.sacc).

The issue is that https://github.com/LSSTDESC/firecrown/blob/master/firecrown/likelihood/gauss_family/statistic/two_point.py#L325 assumes that the ell_or_theta has the same shape as stat, which is not the case because ell_or_theta is more-or-less arbitrary and is only used to compute the theory cells which are then interpolated at the ell of the window function.

I'm also not sure that the scale cuts at https://github.com/LSSTDESC/firecrown/blob/master/firecrown/likelihood/gauss_family/statistic/two_point.py#L349 are correct, since it's not clear to me if ell_or_theta is the tag on the data point in the sacc file or the arbitrary points at which the theory cells get computed.

@marcpaterno @mattkwiecien

[marcpaterno]

We have tried to produce a failure using the SACC file you specified, and have not been able to create one.

Can you please submit a PR on a new branch, containing a minimal SACC file that causes the failure, and a (failing) unit test that reads that SACC file and which provokes the failure?

[marcpaterno]

@tilmantroester I'm tagging you because I neglected to do so earlier, and I suspect you may have not seem our earlier request.

[tilmantroester]

I just got around to create a MWE now. I don't have a minimal sacc file though.

import pytest

import sacc

import firecrown.likelihood.gauss_family.statistic.source.weak_lensing as wl
from firecrown.likelihood.gauss_family.statistic.two_point import TwoPoint
from firecrown.likelihood.gauss_family.gaussian import ConstGaussian
from firecrown.modeling_tools import ModelingTools
from firecrown.likelihood.likelihood import Likelihood

def build_likelihood(build_parameters) -> tuple[Likelihood, ModelingTools]:
    # Load sacc file
    sacc_data = build_parameters["sacc_data"]
    if isinstance(sacc_data, str):
        sacc_data = sacc.Sacc.load_fits(sacc_data)

    tracer = wl.WeakLensing(
        sacc_tracer="src0"
    )

    stat = TwoPoint(
        source0=tracer,
        source1=tracer,
        sacc_data_type="galaxy_shear_cl_ee",
    )

    modeling_tools = ModelingTools()
    likelihood = ConstGaussian(statistics=[stat])

    likelihood.read(sacc_data)

    return likelihood, modeling_tools

SACC_FILE = "../../../forecasting/forecast_inputs/datafiles/sacc_files/data_and_cov/forecast_fid_3x2pt_linear_sys_20_limber.sacc"

def test_broken_window_function():
    with pytest.raises(AssertionError):
        likelihood, modeling_tools = build_likelihood(
            build_parameters=dict(
                sacc_data=SACC_FILE
            )
        )

if __name__ == "__main__":
    test_broken_window_function()

[tilmantroester]

We should distinguish the various ell_or_theta in TwoPoint according to their role. E.g. tags for statistics that are read from the sacc file, points at which the theory gets computed, points at which the theory gets interpolated, etc. These all can have different shapes but use the same name at the moment.

[vitenti]

Hi @tilmantroester , we just released v1.7.2 (to appear on conda-forge soon) containing further refactoring of two point and fixing the window function reading error. There we also included more coherent treatment of ell and theta. Please take a look to see if we can close this issue.

[tilmantroester]

On 1.7.2 it runs now but I get different values for the likelihood than before #395 was merged, so something changed.

[vitenti]

Hi @tilmantroester , we just created a new PR #407 including validation tests comparing the values of Cell with a benchmark. This benchmark was computed using firecrown v1.6.0. I checked and from v1.5.0 to present all agree with this benchmark.

Thus, it seems that the disagreement is coming from elsewhere, please, point out to us the code that is producing different results.

[tilmantroester]

This passes on v1.7.1 (and up to 73af686f050ea60d349a0e6792a4832f2e7f554e) but fails on v1.7.2:

import pytest

import sacc

import pyccl as ccl

import firecrown.likelihood.gauss_family.statistic.source.weak_lensing as wl
from firecrown.likelihood.gauss_family.statistic.two_point import TwoPoint
from firecrown.likelihood.gauss_family.gaussian import ConstGaussian
from firecrown.modeling_tools import ModelingTools
from firecrown.likelihood.likelihood import Likelihood

def build_likelihood(build_parameters) -> tuple[Likelihood, ModelingTools]:
    # Load sacc file
    sacc_data = build_parameters["sacc_data"]
    if isinstance(sacc_data, str):
        sacc_data = sacc.Sacc.load_fits(sacc_data)

    tracer = wl.WeakLensing(
        sacc_tracer="src0"
    )

    stat = TwoPoint(
        source0=tracer,
        source1=tracer,
        sacc_data_type="galaxy_shear_cl_ee",
    )

    modeling_tools = ModelingTools()
    likelihood = ConstGaussian(statistics=[stat])

    likelihood.read(sacc_data)

    return likelihood, modeling_tools

SACC_FILE = "../../../forecasting/forecast_inputs/datafiles/sacc_files/data_and_cov/forecast_fid_3x2pt_linear_sys_20_limber.sacc"

def test_broken_window_function():
    with pytest.raises(AssertionError):
        likelihood, modeling_tools = build_likelihood(
            build_parameters=dict(
                sacc_data=SACC_FILE
            )
        )

def test_likelihood():
    likelihood, modeling_tools = build_likelihood(
        build_parameters=dict(
            sacc_data=SACC_FILE
        )
    )

    ccl_cosmo = ccl.CosmologyVanillaLCDM()

    params = {}
    likelihood.update(params)
    modeling_tools.update(params)
    modeling_tools.prepare(ccl_cosmo)

    log_like = likelihood.compute_loglike(modeling_tools)
    # Compare to 73af686f050ea60d349a0e6792a4832f2e7f554e
    assert abs(-15.014821 - log_like) < 1e-4

if __name__ == "__main__":
    # test_broken_window_function()
    test_likelihood()

[marcpaterno]

@tilmantroester, I assume you're running this program directly, and not using pytest -- because it can never succeed under pytest. Is is only test_likelihood that is failing for you?

[tilmantroester]

Yes, this is about test_likelihood. Which passes under 1.7.1 but fails under 1.7.2.

[vitenti]

Hello @tilmantroester, indeed there was a bug in 1.7.2, it was already fixed in master. I fixed the bug in 1.7.2 and released another bug fix release 1.7.3.

[marcpaterno]

@tilmantroester We have put your test into the firecrown CI, and it is now passing on version 1.7.3 (on conda-forge) and also at the head of the master branch in the repository.

Do you agree this is now solved, so that we can close this issue?

[tilmantroester]

As far as I can tell, this seems to be working as intended again and can be closed.