Skip to content

Conversation

@weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Nov 11, 2025

Let's remove this validator as we have speed up preprocessing.

Greptile Overview

Greptile Summary

Removed validation limiting geometry count in ClipOperation structures to improve performance after preprocessing optimizations.

Key Changes:

  • Commented out _validate_num_geometries validator in Scene class
  • Commented out MAX_GEOMETRY_COUNT constant (5000 limit)
  • Removed imports for ClipOperation, GeometryGroup, flatten_groups, and traverse_geometries that are no longer needed
  • Updated CHANGELOG.md to describe the change

Critical Issues:

  • MAX_GEOMETRY_COUNT is still imported by test files (tests/test_components/test_scene.py:12 and tests/test_components/test_simulation.py:16), causing import errors since it's now commented out
  • Tests test_max_geometry_validation in both test files will fail when trying to reference the commented constant

Confidence Score: 1/5

  • This PR will break existing tests and cannot be merged in its current state
  • The constant MAX_GEOMETRY_COUNT is commented out but still imported by two test files. This will cause immediate import failures when tests run. The tests need to be updated to either remove the import or the constant should remain defined (though unused) for backward compatibility.
  • Pay close attention to tidy3d/components/scene.py where MAX_GEOMETRY_COUNT is commented out, breaking test imports

Important Files Changed

File Analysis

Filename Score Overview
tidy3d/components/scene.py 1/5 Commented out validator and constant; breaks tests that import MAX_GEOMETRY_COUNT; removed now-unused imports
CHANGELOG.md 5/5 Updated changelog entry to reflect validator removal instead of "allowing more geometries"

Sequence Diagram

sequenceDiagram
    participant User
    participant Scene
    participant Validator
    participant Tests

    Note over User,Tests: Before PR (Current Behavior)
    User->>Scene: Create Scene with structures
    Scene->>Validator: _validate_num_geometries()
    Validator->>Validator: Count geometries in ClipOperation
    alt Geometry count > MAX_GEOMETRY_COUNT
        Validator-->>Scene: Raise SetupError
        Scene-->>User: Validation failed
    else Geometry count <= MAX_GEOMETRY_COUNT
        Validator-->>Scene: Validation passed
        Scene-->>User: Scene created successfully
    end
    
    Tests->>Scene: Import MAX_GEOMETRY_COUNT
    Scene-->>Tests: Return constant value (5000)
    
    Note over User,Tests: After PR (New Behavior)
    User->>Scene: Create Scene with structures
    Note over Validator: _validate_num_geometries() commented out
    Scene-->>User: Scene created (no validation)
    
    Tests->>Scene: Import MAX_GEOMETRY_COUNT
    Note over Scene: MAX_GEOMETRY_COUNT commented out
    Scene--xTests: ImportError/AttributeError
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@weiliangjin2021 weiliangjin2021 force-pushed the weiliang/FXC-3648-increase-allowed-structure-count-for-validator-rf branch from d372850 to 56310b3 Compare November 11, 2025 21:06
@github-actions
Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/components/scene.py (100%)

Summary

  • Total: 2 lines
  • Missing: 0 lines
  • Coverage: 100%

@weiliangjin2021 weiliangjin2021 added this pull request to the merge queue Nov 12, 2025
Merged via the queue into develop with commit 3aeee84 Nov 12, 2025
37 checks passed
@weiliangjin2021 weiliangjin2021 deleted the weiliang/FXC-3648-increase-allowed-structure-count-for-validator-rf branch November 12, 2025 17:08
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