fix: improve spatialviz utils quality

[Luodian]

Summary

Follow-up improvements to the SpatialViz task (PR #894):

• Fix wrong exception type: FileExistsError → FileNotFoundError (file doesn't exist, not file exists)
• Use loguru logger: Replace print() with eval_logger for consistent logging across the codebase
• Add type hints: All functions now have proper type annotations
• Fix hidden bug: Missing comma in final_answer_patterns list ("答案" "correct path" was concatenated as single string)
• Remove dead code: Redundant image_path = image_path assignment
• Fix potential bug: Initialize op variable to prevent UnboundLocalError
• Code style: Break long prompt string for 88-char line limit compliance

[claude]

Code Review

This PR makes solid quality improvements to the SpatialViz task utilities. The changes align well with the codebase standards outlined in CLAUDE.md. Here's my detailed review:

✅ Strengths

1. Correct Bug Fixes

   • Exception type fix (line 40): Changing FileExistsError → FileNotFoundError is correct. The code checks if os.path.exists() and raises when the file does NOT exist.
   • Missing comma fix (lines 94-95): Good catch! The original "答案" "correct path" would concatenate into a single string "答案correct path", breaking the pattern matching logic.
   • UnboundLocalError fix (line 76): Initializing op: List[str] = [] prevents potential runtime errors if neither branch assigns to op.
   • Dead code removal (line 37): Removing redundant image_path = image_path improves code clarity.

2. Type Hints

   • All functions now have proper type annotations following CLAUDE.md requirements
   • Type hints are accurate and helpful (e.g., Dict[str, Any], List[Image.Image])
   • Proper use of specific types in line 128-130 for defaultdict tracking

3. Logging Consistency

   • Replacing print() with eval_logger aligns with codebase patterns (verified in worldqa, wild_vision_bench, etc.)
   • Appropriate log levels: debug for routine info, info for results

4. Code Style

   • Multi-line string formatting (lines 46-52) properly handles 88-char line limit
   • Follows PEP 8 and project conventions
   • Clean, readable formatting of long dictionaries and lists

🔍 Minor Observations

1. Potential Logic Issue (lines 97-104)

   When response length is 1, the code extracts [A-D] directly. But when length > 1, it only extracts if a pattern match is found. If no pattern matches, op remains empty [].

   Then at line 106, op = list(set(op)) deduplicates, and line 108 checks len(op) == 1. This means:

   • If no pattern matches in the else branch, op = [], then len(op) == 0, so is_correct = False
   • This might be intentional (fail-safe behavior), but worth confirming this is the desired logic

2. Error Message Clarity (line 40)

   Error message says "image path: {image_path} does not exist."

   Minor: The old message said "video path:" (which was wrong), now says "image path:" (correct). Good fix!

3. Test Coverage

   No tests found for this module. While not required for this PR, adding tests would help prevent regressions, especially for the bug fixes:

   • Test the comma-separated pattern matching
   • Test op initialization edge cases
   • Test exception raising for missing files

🎯 Code Quality Assessment

Aspect	Rating	Notes
Bug Fixes	✅ Excellent	All fixes are correct and meaningful
Type Hints	✅ Excellent	Complete, accurate type annotations
Code Style	✅ Excellent	Follows CLAUDE.md conventions
Logging	✅ Excellent	Consistent with codebase patterns
Performance	✅ Good	No performance concerns introduced
Security	✅ Good	No security issues

📋 Recommendations

Optional improvements (not blockers):

1. Consider adding a test file to cover the fixed bugs
2. Review the pattern matching logic in lines 97-104 to ensure empty op handling is intentional
3. Consider logging the actual response when no patterns match (might help debugging)

✅ Approval

This PR is ready to merge. The improvements are well-executed, and the bugs fixed are real issues that could cause problems in production. The code follows project conventions and improves overall code quality.

[claude]

Code Review

Thank you for this cleanup PR! The improvements significantly enhance code quality and fix several important issues. Here's my detailed review:

✅ Strengths

1. Critical Bug Fixes

   • Fixed concatenated string bug (utils.py:92-93): The missing comma between "答案" and "correct path" would have caused them to concatenate into a single string. Excellent catch!
   • Correct exception type (utils.py:40): FileNotFoundError is indeed the right exception when a file doesn't exist
   • UnboundLocalError prevention (utils.py:74): Initializing op: List[str] = [] prevents potential runtime errors

2. Code Quality Improvements

   • Type hints: All functions now have proper type annotations following PEP 484
   • Logging consistency: Replaced print() with eval_logger for proper logging infrastructure
   • Line length compliance: Properly wrapped long strings to meet the 88-character limit
   • Dead code removal: Removed redundant image_path = image_path assignment

3. Best Practices Adherence

   • Follows CLAUDE.md requirements for type hints and code style
   • Clean, readable code with improved maintainability
   • Proper use of loguru logger with appropriate log levels (debug for debugging, info for results)

🔍 Issues & Suggestions

1. Type Annotation Inconsistency (utils.py:126-128)

The type hints for the defaultdict variables are incorrect:

task_to_eval_samples: Dict[str, List[int]] = defaultdict(list)
category_to_eval_samples: Dict[str, List[int]] = defaultdict(list)
key_to_eval_samples: Dict[str, List[int]] = defaultdict(list)

Issue: These store binary scores (0 or 1) per line 141-147, so List[int] is correct. However, the static type checker may complain because defaultdict(list) creates defaultdict[list], not Dict[str, List[int]].

Recommendation: Use explicit factory or proper typing:

from typing import DefaultDict

task_to_eval_samples: DefaultDict[str, List[int]] = defaultdict(list)
category_to_eval_samples: DefaultDict[str, List[int]] = defaultdict(list)
key_to_eval_samples: DefaultDict[str, List[int]] = defaultdict(list)

2. Missing Test Coverage

Issue: No tests found for this module. The bug fixes (especially the concatenated string) wouldn't have been caught by automated testing.

Recommendation: Add unit tests for:

• spatialviz_doc_to_visual: Test FileNotFoundError handling
• spatialviz_process_results: Test answer extraction with various response formats
• Edge cases: Empty responses, malformed tags, multiple matches

3. Potential Division by Zero (utils.py:150-152)

While utils.py:149 guards against division by zero for overall accuracy, the per-category/task/key accuracies don't have this guard:

task_accuracies = {task: sum(scores) / len(scores) for task, scores in task_to_eval_samples.items()}

Issue: If a defaultdict entry exists but is empty (edge case), this could raise ZeroDivisionError.

Recommendation: Add guards:

task_accuracies = {
    task: sum(scores) / len(scores) if scores else 0 
    for task, scores in task_to_eval_samples.items()
}

4. Error Message Clarity (utils.py:40)

Minor improvement opportunity:

raise FileNotFoundError(f"image path: {image_path} does not exist.")

Suggestion: Include what the user should do:

raise FileNotFoundError(
    f"Image not found at {image_path}. "
    f"Category: {category}, Task: {task}, Level: {level}, ID: {image_id}"
)

5. Log Level Consideration (utils.py:80)

eval_logger.debug("No match for think/answer tags in response")

Consideration: This might be worth logging at warning level since it indicates the model didn't follow the expected format, which could affect accuracy. debug level may hide important issues in production.

📊 Performance Considerations

• Line 104: op = list(set(op)) - Creating a set then converting to list is fine for small lists (A-D options), but unnecessary if you only check len(op) == 1. The deduplication is reasonable for correctness though.
• Logging overhead: The f-string formatting in logging statements will be evaluated even if the log level filters them out. For performance-critical code, use lazy formatting: eval_logger.info("Total: %s", total). However, for this use case (aggregate results called once), current approach is fine.

🔒 Security Considerations

• Path traversal: utils.py:35 constructs file paths from user-provided data. While the snapshot_download likely sanitizes the base directory, ensure that category, task, level, and image_id fields cannot contain path traversal sequences like ../.
• ReDoS risk: The regex patterns at utils.py:68-69 use .*? with re.DOTALL. While non-greedy, extremely large responses could cause performance issues. Consider adding response length limits if not already present.

✨ Overall Assessment

This PR demonstrates excellent attention to detail and significantly improves code quality. The bug fixes are valuable, especially the string concatenation issue. The main areas for improvement are:

1. Fix type annotations to use DefaultDict
2. Add unit tests to prevent regressions
3. Add zero-division guards for aggregate calculations
4. Consider log level for formatting errors

Recommendation: Approve with minor suggested improvements. The current changes are solid and can merge as-is if type checking passes. The suggestions above can be addressed in follow-up PRs if preferred.

Great work! 🎉

[claude]

Code Review - PR #961

Thank you for this quality improvement PR! I've reviewed the changes and here's my feedback:

✅ Positive Changes

SpatialViz Improvements (Primary Focus)

1. Bug Fixes:

   • ✅ Fixed FileExistsError → FileNotFoundError (correct exception type for missing files)
   • ✅ Fixed critical bug: Missing comma in final_answer_patterns list causing "答案" "correct path" to concatenate into a single string
   • ✅ Initialized op variable to prevent potential UnboundLocalError
   • ✅ Removed dead code (image_path = image_path)

2. Code Quality:

   • ✅ Added comprehensive type hints to all functions
   • ✅ Replaced print() with eval_logger for consistent logging (follows codebase patterns)
   • ✅ Split long prompt string for 88-char line limit compliance
   • ✅ Improved error message ("video path" → "image path")

General Code Formatting

• ✅ Applied consistent formatting across 15 files (black/isort)
• ✅ Fixed trailing whitespace and import ordering
• ✅ Standardized quote usage (single → double quotes)

⚠️ Issues & Concerns

1. Large Scope (Major Concern)
This PR touches 15 files with 1367 additions / 409 deletions. According to CLAUDE.md:

"Minimal Changes: Only modify code related to the task at hand"

The PR title says "improve spatialviz utils quality" but includes unrelated formatting changes to:

• amber_g/utils.py (329 lines changed)
• coco_cap_chair/utils.py (902 lines added!)
• groundingme/utils.py (173 lines changed)
• whisper_tt.py, api/metrics.py, and 9 other files

Recommendation: Split this into two PRs:

1. SpatialViz bug fixes and improvements (focused, critical changes)
2. Codebase-wide formatting improvements (separate, reviewable)

2. Missing Test Coverage
Per CLAUDE.md requirements:

"Bug fixes require regression tests"

The PR fixes multiple bugs but doesn't add tests for:

• The missing comma bug in final_answer_patterns
• The FileNotFoundError path
• The op variable initialization fix

3. coco_cap_chair/utils.py - Massive Addition
This file shows +902 lines. Is this:

• New functionality being added?
• Code being moved/refactored?
• Formatting only?

Without context, this is concerning and hard to review.

4. Import Ordering (lmms_eval/api/metrics.py:554)
The imports are reordered but don't follow conventional grouping:

# Comment says "for amber_g" but appears before coco_cap_chair imports
from lmms_eval.tasks.amber_g.utils import (...)
from lmms_eval.tasks.coco_cap_chair.utils import (...)

Consider keeping related imports grouped by their purpose/comment.

🔍 Code-Specific Observations

spatialviz/utils.py:74 - Good defensive coding:

op: List[str] = []  # Initialization prevents UnboundLocalError

spatialviz/utils.py:40 - Good fix:

raise FileNotFoundError(f"image path: {image_path} does not exist.")

This is semantically correct (file doesn't exist, not "file exists").

spatialviz/utils.py:92 - Critical bug fix:

# Before: "答案" "correct path"  # Missing comma = concatenation!
# After:  "答案", "correct path"  # Correct

📋 Recommendations

1. Split the PR: Separate SpatialViz fixes from formatting changes
2. Add Tests: Include regression tests for the bug fixes
3. Explain coco_cap_chair changes: Clarify why 902 lines were added
4. Run Pre-commit Hooks: Ensure uv run ruff format . and uv run ruff check . --fix pass
5. Type Check: Run uv run pyright to verify type annotations

🎯 Summary

Strengths:

• Excellent bug identification (especially the missing comma)
• Good adherence to type hints and logging standards
• Meaningful improvements to code quality

Weaknesses:

• PR scope too broad (mixing critical fixes with formatting)
• Missing test coverage for bug fixes
• Unexplained large additions to coco_cap_chair

Verdict: The SpatialViz improvements are excellent and should be merged, but the PR needs restructuring. Consider splitting into focused PRs that are easier to review and align with the "minimal changes" principle.

---

Note: Please verify all CI checks pass, especially Ruff formatting and Pyright type checking, before merging.

[Luodian]

Retriggering CI

[claude]

Code Review - PR #961: Improve spatialviz utils quality

Thank you for this comprehensive cleanup PR! Overall, this is excellent work that significantly improves code quality across multiple files. Here's my detailed review:

✅ Excellent Changes

spatialviz/utils.py (Primary Focus)

1. Critical Bug Fix - Missing Comma: Fixed the concatenated string bug in final_answer_patterns where "答案" "correct path" became a single string. This was a real bug that could cause parsing issues. Great catch! ✓

2. Correct Exception Type: Changed FileExistsError → FileNotFoundError. This is semantically correct - when a file doesn't exist, we should raise FileNotFoundError. Also fixed the typo "video path" → "image path". ✓

3. Proper Logging: Replaced print("No match for think/answer \n") with eval_logger.debug("No match for think/answer tags in response"). This follows the codebase's logging convention and provides better structured logging. ✓

4. Type Hints: Added comprehensive type annotations (Dict[str, Any], List[Image.Image], etc.) to all functions, improving code clarity and enabling better IDE support. ✓

5. Variable Initialization: Initialize op: List[str] = [] before conditional blocks prevents potential UnboundLocalError if both conditions fail. ✓

6. Dead Code Removal: Removed redundant image_path = image_path assignment. ✓

7. Line Length Compliance: Broke long prompt string to comply with 88-char limit using implicit string concatenation with parentheses. Follows CLAUDE.md guidelines. ✓

Other Files

• amber_g/utils.py: Improved formatting, import organization, and consistency
• api/metrics.py: Better import sorting following Ruff I001
• whisper_tt.py: Formatting improvements and import organization
• Multiple utils files: Consistent formatting and minor improvements

🔍 Minor Observations

1. coco_cap_chair/utils.py (+902 lines): This file has massive formatting changes converting long single-line lists/dicts into multi-line formatted versions. While this improves readability, it significantly increases the diff size. The change is good for maintainability, but verify that:

   • No semantic changes occurred during reformatting
   • All list/dict entries are preserved
   • No accidental duplicates or deletions

2. Import Organization: Several files show import reordering (e.g., api/metrics.py). Ensure uv run ruff check . --fix was run to verify import order follows project standards.

3. Line Length: Some files like utils.py and whisper_tt.py have long lines compressed to single lines. For example:

   raise ImportError(f"Failed to import function '{function_name}' from module '{module_name}'. " f"Tried relative path '{module_path}' and absolute import.") from ex

   While this fits 88 chars, consider if this hurts readability vs multi-line formatting.

✅ Testing & Quality Checks

Before merging, please verify:

1. Run Formatters:

   uv run ruff format .
   uv run ruff check . --fix

2. Type Checking:

   uv run pyright lmms_eval/tasks/spatialviz/utils.py

3. Functional Testing: Verify spatialviz task still works correctly:

   python -m lmms_eval --model <model> --tasks spatialviz --limit 2

4. Verify coco_cap_chair data: Since this file had 900+ lines reformatted, ensure no data was lost during formatting.

📝 Adherence to CLAUDE.md

✅ Following guidelines:

• Line length: 88 chars maximum ✓
• Type hints required ✓
• Using uv for package management ✓
• Code style: PEP 8, f-strings ✓
• Functions are focused and small ✓
• Following existing patterns ✓

🎯 Final Recommendation

APPROVE - This is high-quality cleanup work that:

• Fixes real bugs (missing comma, wrong exception type)
• Improves code quality (type hints, logging)
• Follows project conventions
• Removes dead code

The changes are well-documented in the PR description and align with the repository's development guidelines.

Minor request: Please confirm that all formatters/linters pass and the spatialviz task has been tested before merging.

Great work on catching these issues, especially the subtle missing comma bug! 🎉