Skip to content

Conversation

@SAKavli
Copy link
Contributor

@SAKavli SAKavli commented Sep 30, 2025

Issue
Resolves #my_issue

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

  • PR title captures the intent of the changes, and is fitting for release notes.
  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.
  • Make sure unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

When applicable

  • When there are user facing changes: Updated documentation
  • New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
  • Large PR: Prepare changes in small commits for more convenient review
  • Bug fix: Add regression test for the bug
  • Bug fix: Add backport label to latest release (format: 'backport release-branch-name')

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 30, 2025

CodSpeed Performance Report

Merging #11971 will not alter performance

Comparing SAKavli:investigate-negative-failing-reals-log (a8c812f) with main (2ad6f44)

Summary

✅ 22 untouched

Copy link

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 adds detection logic to identify when the number of successful realizations exceeds the number of starting realizations, which would indicate an internal error in the experiment run tracking.

  • Adds a check after logging experiment results to detect negative failing realizations
  • Triggers a ZeroDivisionError when this impossible condition is detected

f"{len(starting_realizations) - num_successful_realizations}"
)
if len(starting_realizations) - num_successful_realizations < 0:
1 / 0
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The code intentionally triggers a ZeroDivisionError with 1 / 0 to detect a condition that should be impossible. This is a poor error handling approach. Instead, raise a descriptive exception that explains what went wrong.

Suggested fix:

if len(starting_realizations) - num_successful_realizations < 0:
    raise RuntimeError(
        f"Internal error: Number of successful realizations ({num_successful_realizations}) "
        f"exceeds number of starting realizations ({len(starting_realizations)})"
    )
Suggested change
1 / 0
raise RuntimeError(
f"Internal error: Number of successful realizations ({num_successful_realizations}) "
f"exceeds number of starting realizations ({len(starting_realizations)})"
)

Copilot uses AI. Check for mistakes.
Comment on lines +816 to +817
if len(starting_realizations) - num_successful_realizations < 0:
1 / 0
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

This new detection logic for negative failing realizations lacks test coverage. According to the coding guidelines, all new functional paths or behaviors should be covered with unit tests or integration/UI tests.

The test should:

  1. Verify that an exception is raised when num_successful_realizations exceeds len(starting_realizations)
  2. Use a descriptive test name following the test_that_ convention, such as test_that_internal_error_is_raised_when_successful_realizations_exceed_starting_realizations

Copilot generated this review using guidance from repository custom instructions.
f"{len(starting_realizations) - num_successful_realizations}"
)
if len(starting_realizations) - num_successful_realizations < 0:
1 / 0
Copy link

Copilot AI Nov 12, 2025

Choose a reason for hiding this comment

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

The error message produced by triggering 1 / 0 will be a generic ZeroDivisionError: division by zero, which is unclear and unhelpful for users or developers trying to understand what went wrong. The error message should clearly explain that this is an internal inconsistency where the number of successful realizations exceeds the number of starting realizations.

Suggested change
1 / 0
raise ErtRunError(
"Internal inconsistency: number of successful realizations "
"exceeds the number of starting realizations. "
f"Started: {len(starting_realizations)}, "
f"Successful: {num_successful_realizations}."
)

Copilot uses AI. Check for mistakes.
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.

1 participant