Skip to content

Add and test handling of CCL integration errors#535

Merged
marcpaterno merged 3 commits into
masterfrom
handle_ccl_integration_failure
Jul 18, 2025
Merged

Add and test handling of CCL integration errors#535
marcpaterno merged 3 commits into
masterfrom
handle_ccl_integration_failure

Conversation

@marcpaterno
Copy link
Copy Markdown
Collaborator

@marcpaterno marcpaterno commented Jul 14, 2025

Description

CCL will sometimes throw exceptions indicating an error has occurred during integration. Currently this results in Firecrown's likelihood.compute_loglike propagating the exception. This is inconvenient when Firecrown is being used for sampling.

This PR makes the connectors for Firecrown all capture any integration error raised by CCL. Rather than propagating the exception, a log-likelihood of -np.inf is returned.

Type of change

Please delete the bullet items below that do not apply to this pull request.

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

The following checklist will make sure that you are following the code style and
guidelines of the project as described in the
contributing page.

  • I have run bash pre-commit-check and fixed any issues
  • I have added tests that prove my fix is effective or that my feature works
  • I have made corresponding changes to the documentation
  • I have 100% test coverage for my changes (please check this after the CI system has verified the coverage)

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new compute_loglike_for_sampling method to handle CCL integration errors by returning -np.inf instead of propagating exceptions, updates all connectors to use this new method, and adds tests to verify the error-handling behavior.

  • Added compute_loglike_for_sampling to Likelihood to catch CCL integration errors.
  • Updated NumCosmo, CosmoSIS, and Cobaya connectors to call the new sampling-safe method.
  • Added tests in test_likelihood.py for integration-error and non-integration-error handling.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
firecrown/likelihood/likelihood.py Added compute_loglike_for_sampling to swallow integration errors.
firecrown/connector/numcosmo/numcosmo.py Switched compute_loglike to compute_loglike_for_sampling in m2lnL.
firecrown/connector/cosmosis/likelihood.py Switched compute_loglike to compute_loglike_for_sampling in execute.
firecrown/connector/cobaya/likelihood.py Switched compute_loglike to compute_loglike_for_sampling in logp.
tests/likelihood/test_likelihood.py Added tests for sampling-safe loglike and propagation of other errors.
Comments suppressed due to low confidence (3)

firecrown/likelihood/likelihood.py:140

  • The warnings module is used here but not imported; please add import warnings at the top of the file.
                warnings.warn(f"CCL error:\n{e}\nin likelihood, returning -inf")

tests/likelihood/test_likelihood.py:9

  • Docstring contains an extra quote before the closing triple quotes; please remove the stray '"'.
    """A likelihood that always throws a pyccl integration error exception"."""

tests/likelihood/test_likelihood.py:20

  • Docstring contains an extra quote before the closing triple quotes; please remove the stray '"'.
    """A likelihood that always throws a pyccl integration error exception"."""

Comment thread firecrown/likelihood/likelihood.py Outdated
)
def test_other_error_propagates():
likelihood = LikelihoodThatThrowsUnhandledError()
with pytest.raises(RuntimeError):
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test expects RuntimeError, but compute_loglike_for_sampling will rethrow the original pyccl.errors.CCLError. Update the test to catch the correct exception type.

Suggested change
with pytest.raises(RuntimeError):
with pytest.raises(pyccl.errors.CCLError):

Copilot uses AI. Check for mistakes.
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (c12349a) to head (f7fd48d).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #535   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files          49       49           
  Lines        4837     4846    +9     
  Branches      543      544    +1     
=======================================
+ Hits         4837     4846    +9     
Files with missing lines Coverage Δ
firecrown/connector/cobaya/likelihood.py 100.0% <100.0%> (ø)
firecrown/connector/cosmosis/likelihood.py 100.0% <100.0%> (ø)
firecrown/connector/numcosmo/numcosmo.py 100.0% <100.0%> (ø)
firecrown/likelihood/likelihood.py 100.0% <100.0%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@marcpaterno marcpaterno merged commit 5b0c085 into master Jul 18, 2025
12 checks passed
@marcpaterno marcpaterno deleted the handle_ccl_integration_failure branch July 18, 2025 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants