Configurable visualization

[ealt]

No description provided.

[claude]

Code Review: Configurable Visualization System

Summary

This PR introduces a sophisticated, backend-agnostic visualization configuration system based on the Grammar of Graphics. The implementation is well-designed with excellent documentation, but there are several concerns regarding test coverage, type safety, and code quality that need to be addressed before merging.

---

Critical Issues

1. Missing Test Coverage ❌

Severity: HIGH

The PR adds 1,400+ lines of new code across 13 files but includes zero test files. According to CLAUDE.md, new code must meet an 80% coverage threshold via diff-cover.

Required Actions:

• Add comprehensive test suite under tests/visualization/
• Test files needed:
  • test_structured_configs.py - validation logic in __post_init__ methods
  • test_data_pipeline.py - all transform operations, filters, calculations
  • test_data_registry.py - registry resolution, error cases
  • test_altair_renderer.py - chart building, encoding, layering
  • test_plotly_renderer.py - 3D scatter rendering
• Coverage must include:
  • ✅ Happy paths for all transform types (filter, calculate, aggregate, bin, window, fold)
  • ✅ Error cases (missing fields, invalid expressions, unsupported operations)
  • ✅ Validation in __post_init__ methods
  • ✅ Edge cases (empty layers, missing data sources, None handling)

Impact: This is a blocker for merging. CI will fail on the diff-cover check.

---

2. Type Safety Issues ⚠️

Severity: MEDIUM-HIGH

Several issues violate the project's type safety standards (CLAUDE.md requires all code to pass pyright in standard mode):

a) Missing Type Aliases in structured_configs.py:236

The code exports type aliases (BackendType, ChannelType, etc.) from __init__.py but they're not defined in structured_configs.py:

# In __init__.py - these are exported but don't exist!
from .structured_configs import (
    BackendType,    # ❌ Not defined in structured_configs.py
    ChannelType,    # ❌ Not defined
    GeometryType,   # ❌ Not defined
    SelectionType,  # ❌ Not defined
    TransformOp,    # ❌ Not defined
)

Fix Required:
Add type aliases to structured_configs.py:

from typing import Literal

BackendType = Literal["altair", "plotly", "plotnine", "matplotlib"]
ChannelType = Literal["quantitative", "ordinal", "nominal", "temporal"]
GeometryType = Literal["point", "line", "area", "bar", "rect", "rule", "tick", "circle", "square", "text", "boxplot", "errorbar", "errorband"]
SelectionType = Literal["interval", "single", "multi"]
TransformOp = Literal["filter", "calculate", "aggregate", "bin", "window", "fold", "pivot"]

Then update dataclass fields to use these types instead of str.

b) Loose Type Annotations

Several places use str where Literal types should be enforced:

• PlotConfig.backend: str → BackendType
• ChannelAestheticsConfig.type: str | None → ChannelType | None
• GeometryConfig.type: str → GeometryType
• TransformConfig.op: str → TransformOp
• SelectionConfig.type: str → SelectionType
• ScaleConfig.type: str | None → should define ScaleType literal

This would provide compile-time validation and better IDE support.

c) Dynamic Module Import in altair_renderer.py:75-80

The _import_altair() function returns untyped alt module, making the entire renderer untyped:

def _import_altair():  # ❌ Missing return type
    try:
        import altair as alt  # type: ignore import-not-found  # ❌ Untyped module

Recommendation: Add a try-import at module level with proper typing, or add explicit return type annotation.

---

3. Security Concerns ⚠️

Severity: MEDIUM

The CALC_ENV dictionary in data_pipeline.py:19-28 exposes powerful functions to eval/query:

CALC_ENV = {
    "np": np,      # ⚠️ Entire numpy module
    "pd": pd,      # ⚠️ Entire pandas module
    "math": math,  # ⚠️ Entire math module
    ...
}

Issues:

1. Allows arbitrary code execution via df.eval(expr, local_dict=CALC_ENV) (line 117)
2. Exposes entire modules rather than specific safe functions
3. No sanitization or validation of expressions before eval

Recommendation:

• Document security model clearly (trusted configs only? sandboxed?)
• Consider allowlisting specific functions only (not entire modules)
• Add expression validation/sanitization if configs come from untrusted sources
• At minimum, add a security note in the docstring

---

Code Quality Issues

4. Inconsistent Error Handling 🔧

Severity: LOW-MEDIUM

Error handling is inconsistent across modules:

data_registry.py:29:

except KeyError as exc:  # pragma: no cover - simple error wrapper
    raise ValueError(...)

Uses # pragma: no cover to skip coverage, but according to CLAUDE.md, all new code should be tested.

Recommendation: Remove # pragma: no cover and add proper test coverage for error paths.

---

5. Code Smells 🔧

a) Magic Strings (altair_renderer.py:94-99)

mark_name = f"mark_{geometry.type}"
if not hasattr(chart, mark_name):
    raise ConfigValidationError(...)
mark_fn = getattr(chart, mark_name)

Using hasattr/getattr with string interpolation is fragile. Consider a mapping dict or match statement.

b) Repeated Code Patterns

The channel encoding logic in altair_renderer.py:102-113 could be DRYed up with a loop over channel definitions.

c) Too Many Instance Attributes (structured_configs.py:33)

@dataclass
class TransformConfig:  # pylint: disable=too-many-instance-attributes

This pylint disable suggests the class might benefit from decomposition. Consider separate dataclasses for different transform types.

---

Performance Considerations

6. DataFrame Copying ⚡

Severity: LOW

Heavy use of .copy() in data pipeline (lines 38, 46, 67, 76, 85, etc.) could impact performance with large datasets.

Recommendation:

• Document that copying is intentional for immutability
• Consider copy-on-write optimizations for very large datasets
• Benchmark with realistic data sizes

---

Documentation & Best Practices

7. Excellent Documentation ✅

Strengths:

• docs/visualization.md is comprehensive and well-structured
• Docstrings follow Google style convention
• Clear separation of concerns between modules
• Good use of type hints (despite issues noted above)

8. Good Architecture ✅

Strengths:

• Clean separation: configs → data pipeline → renderers
• Protocol-based design for DataRegistry
• Backend-agnostic IR (intermediate representation)
• Extensible for future backends

9. Missing Items 📝

According to the design doc:

• LabelConfig is defined but not implemented in renderers (noted with LOGGER.info)
• FacetConfig.wrap not implemented (raises error in Altair)
• Plotly renderer is prototype-only (single layer, point geometry)

These are acceptable for a draft PR but should be tracked.

---

Style & Conventions

10. Mostly Compliant with CLAUDE.md ✅

• ✅ Line length within 120 chars
• ✅ Type annotations present
• ✅ Google-style docstrings
• ✅ Functional patterns used appropriately
• ✅ No inline comments (code is self-documenting)
• ⚠️ Missing type aliases exported in __init__.py

---

Recommendations

Must Fix Before Merge:

1. Add comprehensive test suite (80%+ coverage via diff-cover)
2. Define and use type aliases instead of bare str types
3. Fix imports in __init__.py to match actual definitions

Should Fix Before Merge:

4. Add security documentation for eval-based expression system
5. Fix type annotations in _import_altair() and renderer functions
6. Remove # pragma: no cover and add proper error path tests

Nice to Have:

7. Refactor TransformConfig to reduce instance attributes
8. Add benchmarks for large DataFrame operations
9. Consider mapping dict instead of hasattr/getattr pattern

---

Example Test Structure

# tests/visualization/test_data_pipeline.py
import pandas as pd
import pytest
from simplexity.visualization.data_pipeline import apply_transforms
from simplexity.visualization.structured_configs import TransformConfig

def test_calculate_transform():
    df = pd.DataFrame({"x": [1, 2, 3]})
    transform = TransformConfig(op="calculate", as_field="y", expr="x * 2")
    result = apply_transforms(df, [transform])
    assert list(result["y"]) == [2, 4, 6]

def test_calculate_transform_missing_field_raises():
    df = pd.DataFrame({"x": [1, 2, 3]})
    transform = TransformConfig(op="calculate", as_field="", expr="x * 2")
    with pytest.raises(ConfigValidationError, match="as_field is required"):
        apply_transforms(df, [transform])

---

Overall Assessment

Strengths:

• Excellent design and architecture
• Comprehensive documentation
• Clean, readable code
• Good separation of concerns

Blockers:

• ❌ Zero test coverage (fails CI requirement)
• ❌ Missing type alias definitions (import errors)

Concerns:

• ⚠️ Security implications of eval-based expressions
• ⚠️ Type safety gaps

Verdict: Request Changes

This is high-quality work with strong architecture, but it cannot merge without tests. The type alias issue will cause import errors. Once tests are added and type issues resolved, this will be an excellent addition to the codebase.

---

Commands to Run Before Merge

# 1. Verify linting
uv run --extra dev ruff check simplexity/visualization/

# 2. Verify formatting
uv run --extra dev ruff format --check simplexity/visualization/

# 3. Verify type checking
uv run --extra dev --extra pytorch pyright simplexity/visualization/

# 4. Run tests
uv run --extra dev --extra pytorch pytest tests/visualization/

# 5. Check diff coverage (must be >= 80%)
uv run --extra dev --extra pytorch pytest --cov-fail-under=0
uv run --extra dev diff-cover coverage.xml --compare-branch=origin/main --fail-under=80

Great work on this feature! Looking forward to seeing the tests.

[claude]

PR Review: Configurable Visualization System

Overview

This PR introduces a comprehensive, backend-agnostic visualization configuration system based on the Grammar of Graphics. The design is well-thought-out and follows good software engineering principles. Below is my detailed review.

---

✅ Strengths

Architecture & Design

• Excellent separation of concerns: The abstraction between config schema, data pipeline, and renderers is clean and extensible
• Backend-agnostic design: PlotConfig serves as a proper intermediate representation that different backends can consume
• Grammar of Graphics alignment: The schema properly models the core concepts (data → transforms → aesthetics → geometry → guides)
• Protocol-based data registry: Good use of Python protocols for flexible data source resolution

Code Quality

• Type annotations: Comprehensive type hints throughout, enabling strong static analysis
• Validation: Good use of __post_init__ for config validation with clear error messages
• Functional style: Appropriate for JAX ecosystem, pure functions for transforms
• Documentation: Extensive design doc (docs/visualization.md) and inline docstrings following Google style

Implementation

• Data pipeline separation: data_pipeline.py cleanly separates data prep from rendering
• Error handling: Appropriate use of ConfigValidationError with descriptive messages
• Demonstration: Two working examples showing both programmatic and YAML-driven usage

---

⚠️ Issues & Recommendations

🔴 CRITICAL: Security Concerns

1. Unsafe eval() usage in data_pipeline.py:117

```python
result[target] = result.eval(expr, engine="python", local_dict=CALC_ENV)
```

Risk: While CALC_ENV restricts available functions, pandas .eval() with engine="python" still allows arbitrary Python execution within the restricted namespace. This could be exploited if user-provided YAML configs contain malicious expressions.

Recommendations:

• Add explicit input validation/sanitization for expressions
• Consider using pandas' numexpr engine (default) which is safer and faster
• Document security constraints clearly in the design doc
• Add warnings about untrusted config files
• Consider implementing an expression allowlist or AST validation

2. pandas .query() with user input (data_pipeline.py:79)

Similar concerns apply to filter expressions. Consider validating filter syntax before execution.

🟡 Test Coverage

Missing test coverage for the visualization module

I searched for test files and found none for the new visualization functionality. According to CLAUDE.md:

• New code requires minimum 80% coverage (enforced via diff-cover)
• This PR will likely fail the CI coverage check

Required tests:

• Unit tests for all renderers (altair_renderer.py, plotly_renderer.py)
• Transform validation and execution tests (data_pipeline.py)
• Config validation tests (structured_configs.py)
• Data registry tests (data_registry.py)
• Integration tests for end-to-end rendering
• Edge cases: empty data, missing fields, invalid configs, etc.

Recommendation: Add comprehensive pytest tests in tests/visualization/ before merging.

🟡 Type Safety Issues

Type annotations could be stricter

```python

structured_configs.py - using strings instead of Literal types

type: str # Should be: type: GeometryType (where GeometryType is Literal[...])
op: str # Should be: op: TransformOp
```

While type aliases are defined (GeometryType, TransformOp, BackendType, etc.), they're not actually used in the dataclass fields. This reduces type safety.

Recommendation: Use the Literal type aliases directly in dataclass fields for better validation:
```python
@DataClass
class GeometryConfig:
type: GeometryType # Instead of str
...
```

🟡 Performance Considerations

1. DataFrame copying

Multiple .copy() calls in the data pipeline could be expensive for large datasets:

• data_pipeline.py:38: df = resolve_data_source(...).copy()
• data_pipeline.py:66: df = plot_df.copy()
• data_pipeline.py:76, 85: Additional copies in apply_filters/transforms

Recommendation: Consider copy-on-write or in-place operations where safe, especially for read-only transforms.

2. Transform efficiency

Some transforms could leverage pandas optimizations:

• Window functions could use pandas' optimized rolling/expanding methods
• Consider vectorization opportunities

🟠 Code Quality & Style

1. Incomplete type annotations in __init__.py

The type aliases are exported but not actually enforced:
```python

These are just strings at runtime, not enforced types

BackendType = Literal["altair", "plotly"]
```

2. Pylint disables

Several pylint: disable comments:

• structured_configs.py:33: too-many-instance-attributes - Consider if this indicates a need to break down classes
• Multiple instances suggest the dataclasses might benefit from refactoring

3. Missing __all__ in some modules

Not all modules define __all__, which could lead to namespace pollution (though this is minor).

4. Line length

Some lines in visualization_3d_demo.py approach or exceed 120 characters. Run ruff format to ensure consistency.

🟢 Minor Issues

1. Incomplete implementation warnings

Good that the code logs warnings for unimplemented features:

• altair_renderer.py:216: Labels not yet implemented
• plotly_renderer.py:114: Labels not yet implemented
• altair_renderer.py:197: Facet wrap not implemented

Consider creating GitHub issues for these TODOs.

2. Documentation inconsistency

The design doc is excellent but very long (1057 lines). Consider:

• Breaking into multiple focused docs
• Adding a quick-start guide
• Including architecture diagrams

3. Examples could use type hints

The demo scripts lack some type annotations (e.g., function return types).

4. Error messages could include config path

When validation fails in nested configs, it's hard to trace which YAML field caused the error. Consider including the config path in error messages.

---

📋 Checklist Against CLAUDE.md

Requirement	Status	Notes
Line length ≤ 120 chars	⚠️	Mostly compliant, check examples
Type annotations	✅	Present but could be stricter
Google-style docstrings	✅	Good coverage
Ruff formatting	❓	Should verify with ruff format --check
Ruff linting	❓	Should verify with ruff check
Pyright type checking	❓	Should verify
Test coverage ≥ 80%	❌	No tests found - will fail CI
Functional style	✅	Appropriate pure functions
Security	❌	eval() usage needs attention

---

🎯 Recommendations Summary

Before Merge (Blockers)

1. Add comprehensive test coverage (80%+ required by CI)
2. Address security concerns with eval/query expressions
3. Run and pass all CI checks:
   • uv run --extra dev ruff check
   • uv run --extra dev ruff format --check
   • uv run --extra dev --extra pytorch pyright
   • uv run --extra dev --extra pytorch pytest
   • uv run --extra dev diff-cover coverage.xml --compare-branch=origin/main --fail-under=80

High Priority

4. Use Literal type aliases in dataclass fields for better type safety
5. Add security documentation and input validation
6. Optimize DataFrame copying in data pipeline

Nice to Have

7. Break down long dataclasses if they grow further
8. Add GitHub issues for TODOs (labels, facet wrap, etc.)
9. Consider splitting design doc into focused sections
10. Add architecture diagram to documentation

---

🏆 Conclusion

This is a well-architected feature with excellent design principles. The Grammar of Graphics approach is exactly right for this use case, and the backend abstraction is clean and extensible.

However, this PR cannot be merged in its current state due to:

1. Missing test coverage (will fail CI)
2. Security concerns with expression evaluation

Once these blockers are addressed, this will be an excellent addition to the codebase. The foundation is solid and the design is future-proof.

Recommendation: Mark as draft, add tests and security fixes, then re-request review.

---

📚 References

• CLAUDE.md coverage requirements (simplexity/visualization/init.py:1)
• eval() usage (simplexity/visualization/data_pipeline.py:117)
• query() usage (simplexity/visualization/data_pipeline.py:79)
• Type definitions (simplexity/visualization/structured_configs.py)

[claude]

Pull Request Review: Configurable Visualization System

Summary

This PR introduces a comprehensive, backend-agnostic visualization configuration system following the Grammar of Graphics principles. The implementation is well-designed with excellent documentation, though there are critical gaps in test coverage that must be addressed before merging.

---

Code Quality ⭐⭐⭐⭐☆ (4/5)

Strengths

1. Excellent Architecture

• Clean separation of concerns: configs → data pipeline → renderers
• Backend-agnostic design with pluggable renderers (Altair, Plotly)
• Protocol-based DataRegistry allows flexible data source management
• Well-structured dataclasses following Hydra patterns

2. Strong Type Safety

• Comprehensive type annotations throughout
• Validation in __post_init__ methods (e.g., ChannelAestheticsConfig, TransformConfig)
• Custom exception ConfigValidationError for clear error messages
• Proper use of Protocol for registry interface

3. Code Style Compliance

• Follows project conventions (120 char lines, type hints, Google docstrings)
• Clean functional patterns appropriate for JAX ecosystem
• Proper use of pylint disable comments with justification
• Good use of field(default_factory=...) for mutable defaults

4. Documentation

• Exceptional design document (docs/visualization.md) with 1000+ lines
• Clear module docstrings explaining backend-agnostic intent
• Inline validation error messages are descriptive

Issues & Concerns

1. CRITICAL: Zero Test Coverage ❌

The PR adds ~2400 lines of code with zero tests. This is unacceptable per CLAUDE.md:

simplexity/visualization/
├── altair_renderer.py (227 lines) - NO TESTS
├── data_pipeline.py (195 lines) - NO TESTS  
├── data_registry.py (40 lines) - NO TESTS
├── plotly_renderer.py (159 lines) - NO TESTS
└── structured_configs.py (237 lines) - NO TESTS

Per project guidelines:

• New code must meet 80% coverage (enforced via diff-cover)
• This PR will fail the coverage check in CI

Required Test Coverage:

# tests/visualization/test_structured_configs.py
- Test validation in __post_init__ methods
- Test ChannelAestheticsConfig field/value mutual exclusion
- Test TransformConfig validation for each op type

# tests/visualization/test_data_pipeline.py
- Test materialize_data with filters and column selection
- Test all transform operations (filter, calculate, aggregate, bin, window, fold)
- Test expression normalization
- Test error handling for missing columns/invalid expressions
- Test CALC_ENV security (ensure eval is properly scoped)

# tests/visualization/test_altair_renderer.py
- Test build_altair_chart with various layer configurations
- Test geometry type mapping
- Test channel encoding for all aesthetic types
- Test selection parameter building
- Test faceting
- Test error handling for invalid configs

# tests/visualization/test_plotly_renderer.py
- Test 3D scatter rendering
- Test opacity validation
- Test required field validation

# tests/visualization/test_data_registry.py
- Test DictDataRegistry get/error cases
- Test resolve_data_source with both registry types

2. Security Concern: Expression Evaluation 🔐

Location: simplexity/visualization/data_pipeline.py:79,117

result = result.query(norm_expr, engine="python", local_dict=CALC_ENV)
result[target] = result.eval(expr, engine="python", local_dict=CALC_ENV)

Risk: Using eval() and query() with user-provided expressions

Current Mitigation:

• Expressions are scoped to CALC_ENV (contains np, pd, math, common functions)
• engine="python" is more restrictive than default

Recommendation:

• Document that configs should come from trusted sources only
• Consider adding expression validation/sanitization
• Add security note to docs/visualization.md
• Include test cases that verify malicious expressions are blocked

3. Missing Error Handling

simplexity/visualization/data_pipeline.py:29 - KeyError pragma comment suggests incomplete error path:

except KeyError as exc:  # pragma: no cover - simple error wrapper
    raise ValueError(f"Data source '{source_name}' is not registered") from exc

This should be tested, not skipped with pragma.

4. Code Smell: Too Many Instance Attributes

Multiple classes use # pylint: disable=too-many-instance-attributes:

• TransformConfig (13 attributes)
• ChannelAestheticsConfig (11 attributes)
• AestheticsConfig (10 attributes)

While this is acceptable for configuration dataclasses, consider:

• Breaking TransformConfig into per-operation subclasses
• Using discriminated unions (Python 3.10+)

5. Incomplete Implementation

simplexity/visualization/altair_renderer.py:197

if facet_cfg.wrap:
    raise ConfigValidationError("FacetConfig.wrap is not yet implemented...")

Either implement or remove from schema.

---

Performance Considerations ⚡

Data Copying:
Multiple .copy() calls in data pipeline could be expensive for large DataFrames:

# data_pipeline.py
df = resolve_data_source(data_cfg.source, data_registry).copy()  # Line 38
result = df.copy()  # Line 76, 85, 116, etc.

Recommendation:

• Document memory implications for large datasets
• Consider copy-on-write mode (pandas 2.0+)
• Add performance tests with realistic data sizes

---

CI/CD Changes ✅

Changes to .github/workflows/simplexity.yaml:

• Adds --extra altair to pyright and pytest commands
• Necessary for type checking new visualization code
• Good: Ensures CI catches import errors

---

Potential Bugs 🐛

1. Unsafe Dictionary Unpacking

simplexity/visualization/altair_renderer.py:157-163,166-168

def _scale_to_alt(cfg: ScaleConfig):
    kwargs = {k: v for k, v in vars(cfg).items() if v is not None}
    return alt.Scale(**kwargs)

If ScaleConfig adds new fields not supported by alt.Scale, this will raise TypeError.

Recommendation: Explicitly map known fields

2. Missing Validation

LayerConfig.geometry has default but type isn't validated:

geometry: GeometryConfig = field(default_factory=lambda: GeometryConfig(type="point"))

Should validate type is in allowed set.

---

Best Practices Violations

1. Missing __all__ Exports

Only data_pipeline.py exports __all__. Add to other modules for clarity.

2. Inconsistent Error Messages

Some use f-strings, others use concatenation. Standardize on f-strings.

3. Magic Strings

_CHANNEL_CLASS_MAP hardcodes Altair class names. Consider making this more robust with getattr error handling.

---

Recommendations

Must Fix Before Merge

1. Add comprehensive test suite targeting 80%+ coverage
2. Document security implications of expression evaluation
3. Test error paths currently marked with pragma: no cover
4. Run diff-cover to verify new code meets coverage threshold:

   uv run --extra dev --extra pytorch pytest
   uv run --extra dev diff-cover coverage.xml --compare-branch=origin/main --fail-under=80

Should Fix

5. Implement or remove FacetConfig.wrap functionality
6. Add validation for geometry types and other enums
7. Add __all__ exports to all modules
8. Consider performance implications of data copying

Nice to Have

9. Add integration tests using example configs from examples/configs/visualization/
10. Document memory usage for large datasets
11. Add performance benchmarks for rendering pipeline
12. Consider breaking TransformConfig into subclasses

---

Security Assessment 🔒

Risk Level: MEDIUM

• Expression evaluation via eval()/query() with user input
• Mitigated by scoped environment and Python engine
• Recommendation: Add explicit documentation that configs must come from trusted sources

---

Final Verdict

This PR demonstrates excellent design and architecture with a well-thought-out abstraction layer. The code quality is high and follows project conventions.

However, it cannot be merged without addressing the critical test coverage gap. The project enforces 80% coverage for new code via diff-cover, and this PR currently has 0% coverage.

Action Items:

1. ✅ Keep the excellent design and implementation
2. ❌ Add comprehensive test suite (blocking)
3. ⚠️ Document security considerations
4. ⚠️ Address error handling gaps

Recommendation: Request changes - add tests before approval.

---

Positive Notes 🎉

Despite the coverage issues, this is a well-architected addition to Simplexity:

• The Grammar of Graphics approach is elegant and extensible
• Backend abstraction allows future flexibility
• Documentation is exceptional
• Code is clean, typed, and maintainable

With proper test coverage, this will be a valuable feature!