fix: raise error when _finalized simulation fails

[dbochkov-flexcompute]

forgot that log.error() doesn't raise an actual error

Greptile Overview

Updated On: 2025-10-20 23:29:22 UTC

Greptile Summary

This PR fixes a critical bug in simulation validation where log.error() was incorrectly assumed to raise an exception. The _validate_finalized() method in Simulation now properly captures and re-raises exceptions after logging, ensuring that validation failures when adding mode source PEC frames are surfaced to users rather than silently ignored. The corresponding test was updated to call _validate_finalized() explicitly instead of accessing the _finalized property directly, ensuring the validation logic is properly exercised. This change aligns with the codebase's error handling pattern where validation failures must raise exceptions, not just log messages, particularly important for pre-upload validation that prevents invalid simulations from being submitted.

Important Files Changed

Changed Files

Filename	Score	Overview
tidy3d/components/simulation.py	5/5	Fixed _validate_finalized() to properly capture and re-raise exceptions after logging, ensuring validation errors are not silently ignored
tests/test_components/test_source_frames.py	4/5	Updated test to call _validate_finalized() method instead of accessing _finalized property; inconsistency remains on line 42 where _finalized is still accessed directly

Confidence score: 4/5

• This PR fixes a critical validation bug with minimal risk, though one test inconsistency remains unaddressed
• Score reflects the correct fix to the core issue but potential incompleteness in test coverage (line 42 in test file still accesses _finalized directly without validation, which may or may not be intentional)
• Pay close attention to tests/test_components/test_source_frames.py line 42 to determine if the direct _finalized access should also be updated to _validate_finalized()

Sequence Diagram

sequenceDiagram
    participant Test as test_source_absorber_frames
    participant Simulation as Simulation
    participant Log as log
    participant Exception as ValidationError

    Note over Test: Create bad_sim with conflicting<br/>mode source frame and projection monitor
    
    Test->>Simulation: bad_sim._validate_finalized()
    activate Simulation
    
    Simulation->>Simulation: try: _ = self._finalized
    activate Simulation
    
    Note over Simulation: Validation fails due to<br/>incompatible components
    
    Simulation->>Exception: Exception raised
    deactivate Simulation
    
    Simulation->>Log: log.error("Simulation fails after<br/>requested mode source PEC frames<br/>are added. Please inspect '._finalized'.")
    
    Simulation->>Exception: raise e
    deactivate Simulation
    
    Exception-->>Test: ValidationError raised
    
    Note over Test: pytest.raises(ValidationError)<br/>assertion passes

Loading

Context used:

• Rule from dashboard - Use log.error instead of assert statements for error handling in production code. (source)

[greptile-apps]

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[github-actions]

Diff Coverage

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

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

Summary

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

[momchil-flex]

Yeah it's kind of the other way round, if you raise a Tidy3dError subclass, it will also get log.error-ed before exiting.

Do you want to also add anything about the frame = {} in this PR?

[caseyflex]

Why not just allow the original exception to be raised? or raise a new exception from it? What was the original reason for the try-catch logic?

[dbochkov-flexcompute]

Do you want to also add anything about the frame = {} in this PR?

Actually, it seems like a common thing throughout tidy3d: setting {} in json to fields that are None by default results in initializing those fields with a class instance (for example, nonlinear_spec, modulation_spec, etc). Probably we need to come up with a way to prevent that from happening on the tidy3d model base level

[dbochkov-flexcompute]

Why not just allow the original exception to be raised? or raise a new exception from it? What was the original reason for the try-catch logic?

to add the additional info that simulation fails after trying to add those auxiliary structures. For example:

ERROR: 2 different mediums detected on plane intersecting a        
DiffractionMonitor. Plane must be homogeneous.                     
WARNING: Could not execute validator 'check_fixed_angle_components'
because field 'monitors' failed validation.                        
ERROR: Simulation fails after requested mode source PEC frames are 
added. Please inspect '._finalized'. 

instead of just

ERROR: 2 different mediums detected on plane intersecting a        
DiffractionMonitor. Plane must be homogeneous.                     
WARNING: Could not execute validator 'check_fixed_angle_components'
because field 'monitors' failed validation.

[caseyflex]

@dbochkov-flexcompute what about this:

except e:
    raise Exception(msg) from e

[dbochkov-flexcompute]

@dbochkov-flexcompute what about this:

except e:
    raise Exception(msg) from e

Works too, just doesn't log the error message. I don't remember to what extent it is import for propagating error information in GUI. I will switch to raise Tidy3dError(msg) from e, which does log and errors, but I don't really have a strong preference which way to go

[momchil-flex]

Seems good to me, we merge, @caseyflex ?

[caseyflex]

Seems good to me, we merge, @caseyflex ?

Sounds good to me