Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .pr_agent.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,12 @@
pr_commands = ["/describe", "/review", "/improve"]

[pr_reviewer] # (all fields optional)
num_max_findings = 5 # how many items to surface
num_max_findings = 7 # how many items to surface
require_tests_review = true
extra_instructions = """
Focus on duplicate code, the possibility of bugs, and if the PR added appropriate tests if it added a simulation feature.
"""

[pr_code_suggestions]
commitable_code_suggestions = false # purely advisory, no write ops
apply_suggestions_checkbox = false # hides the “Apply/Chat” boxes
commitable_code_suggestions = true
apply_suggestions_checkbox = true
83 changes: 76 additions & 7 deletions toolchain/mfc/packer/tol.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,92 @@ def compare(candidate: Pack, golden: Pack, tol: Tolerance) -> typing.Tuple[Error
error = compute_error(cVal, gVal)
avg_err.push(error)

def raise_err(msg: str):
def raise_err_with_max_diagnostics(msg: str):
# Find maximum errors across ALL files for diagnostics
max_errors = find_maximum_errors(candidate, golden)
diagnostic_msg = format_diagnostic_message(max_errors)

return None, f"""\
Variable n°{valIndex+1} (1-indexed) in {gFilepath} {msg}:
- Candidate: {cVal}
- Golden: {gVal}
- Error: {error}
- Tolerance: {tol}
- Tolerance: {tol}{diagnostic_msg}
"""

if math.isnan(gVal):
return raise_err("is NaN in the golden file")

return raise_err_with_max_diagnostics("is NaN in the golden file")
if math.isnan(cVal):
return raise_err("is NaN in the pack file")

return raise_err_with_max_diagnostics("is NaN in the pack file")
if not is_close(error, tol):
return raise_err("is not within tolerance")
return raise_err_with_max_diagnostics("is not within tolerance")

# Return the average relative error
return avg_err.get(), None


def format_diagnostic_message(max_errors: typing.Tuple[typing.Optional[typing.Tuple[str, int, float, float, float, float]], typing.Optional[typing.Tuple[str, int, float, float, float, float]]]) -> str:
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

The function signature uses deeply nested tuple types that are difficult to read and maintain. Consider defining named tuple types or dataclasses for the error information to improve code clarity.

Copilot uses AI. Check for mistakes.
"""Format the diagnostic message showing maximum errors."""
max_abs_info, max_rel_info = max_errors
diagnostic_msg = ""

if max_abs_info:
filepath, var_idx, golden_val, candidate_val, abs_error, rel_error = max_abs_info
rel_error_str = f"{rel_error:.2E}" if not math.isnan(rel_error) else "NaN"
diagnostic_msg += f"\n\nDiagnostics - Maximum absolute error across ALL files:\n" \
f" - File: {filepath}\n" \
f" - Variable n°{var_idx+1}\n" \
f" - Candidate: {candidate_val}\n" \
f" - Golden: {golden_val}\n" \
f" - Absolute Error: {abs_error:.2E}\n" \
f" - Relative Error: {rel_error_str}"

if max_rel_info:
filepath, var_idx, golden_val, candidate_val, rel_error, abs_error = max_rel_info
diagnostic_msg += f"\n\nDiagnostics - Maximum relative error across ALL files:\n" \
f" - File: {filepath}\n" \
f" - Variable n°{var_idx+1}\n" \
f" - Candidate: {candidate_val}\n" \
f" - Golden: {golden_val}\n" \
f" - Relative Error: {rel_error:.2E}\n" \
f" - Absolute Error: {abs_error:.2E}"

return diagnostic_msg


def find_maximum_errors(candidate: Pack, golden: Pack) -> typing.Tuple[typing.Optional[typing.Tuple[str, int, float, float, float, float]], typing.Optional[typing.Tuple[str, int, float, float, float, float]]]:
"""
Scan all files to find the maximum absolute and relative errors.
Returns tuple of:
- max_abs_info: (filepath, var_index, golden_val, candidate_val, absolute_error, relative_error)
- max_rel_info: (filepath, var_index, golden_val, candidate_val, relative_error, absolute_error)
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

The return type annotation uses the same deeply nested tuple structure as format_diagnostic_message. This complex type signature makes the code harder to understand and maintain. Consider using named tuples or dataclasses.

Suggested change
- max_rel_info: (filepath, var_index, golden_val, candidate_val, relative_error, absolute_error)
def format_diagnostic_message(max_errors: Tuple[Optional[ErrorInfo], Optional[ErrorInfo]]) -> str:
"""Format the diagnostic message showing maximum errors."""
max_abs_info, max_rel_info = max_errors
diagnostic_msg = ""
if max_abs_info:
rel_error_str = f"{max_abs_info.relative_error:.2E}" if not math.isnan(max_abs_info.relative_error) else "NaN"
diagnostic_msg += f"\n\nDiagnostics - Maximum absolute error across ALL files:\n" \
f" - File: {max_abs_info.filepath}\n" \
f" - Variable n°{max_abs_info.var_index+1}\n" \
f" - Candidate: {max_abs_info.candidate_val}\n" \
f" - Golden: {max_abs_info.golden_val}\n" \
f" - Absolute Error: {max_abs_info.absolute_error:.2E}\n" \
f" - Relative Error: {rel_error_str}"
if max_rel_info:
diagnostic_msg += f"\n\nDiagnostics - Maximum relative error across ALL files:\n" \
f" - File: {max_rel_info.filepath}\n" \
f" - Variable n°{max_rel_info.var_index+1}\n" \
f" - Candidate: {max_rel_info.candidate_val}\n" \
f" - Golden: {max_rel_info.golden_val}\n" \
f" - Relative Error: {max_rel_info.relative_error:.2E}\n" \
f" - Absolute Error: {max_rel_info.absolute_error:.2E}"
return diagnostic_msg
def find_maximum_errors(candidate: Pack, golden: Pack) -> Tuple[Optional[ErrorInfo], Optional[ErrorInfo]]:
"""
Scan all files to find the maximum absolute and relative errors.
Returns tuple of:
- max_abs_info: ErrorInfo or None
- max_rel_info: ErrorInfo or None

Copilot uses AI. Check for mistakes.
"""
max_abs_error = -1.0
max_abs_info = None

max_rel_error = -1.0
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

Using -1.0 as an initial value for maximum error tracking is unclear and could be problematic if all errors are actually negative (though unlikely in this context). Consider using float('-inf') or None with explicit checks for better clarity.

Suggested change
max_rel_error = -1.0
max_abs_error = float('-inf')
max_abs_info = None
max_rel_error = float('-inf')

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Aug 18, 2025

Choose a reason for hiding this comment

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

Same issue as with max_abs_error - using -1.0 as initial value is unclear. Consider using float('-inf') or None with explicit checks for better clarity.

Suggested change
max_rel_error = -1.0
max_abs_error = float('-inf')
max_abs_info = None
max_rel_error = float('-inf')

Copilot uses AI. Check for mistakes.
max_rel_info = None

for gFilepath, gEntry in golden.entries.items():
cEntry = candidate.find(gFilepath)
if cEntry is None:
continue

for valIndex, (gVal, cVal) in enumerate(zip(gEntry.doubles, cEntry.doubles)):
# Skip NaN values in golden or candidate
if math.isnan(gVal) or math.isnan(cVal):
continue

error = compute_error(cVal, gVal)

# Track maximum absolute error
if error.absolute > max_abs_error:
max_abs_error = error.absolute
max_abs_info = (gFilepath, valIndex, gVal, cVal, error.absolute, error.relative)

# Track maximum relative error (only if it's not NaN)
if not math.isnan(error.relative) and error.relative > max_rel_error:
max_rel_error = error.relative
max_rel_info = (gFilepath, valIndex, gVal, cVal, error.relative, error.absolute)

return max_abs_info, max_rel_info
Loading