SCRF-1630-Modeler validation and sim_dict performance improvements

[momchil-flex]

Separated these updates in a standalone PR.

Greptile Overview

Updated On: 2025-11-10 18:14:34 UTC

Greptile Summary

This PR optimizes TerminalComponentModeler performance by deferring validation until necessary. The changes skip validation (validate=False) and deep copying (deep=False) during intermediate simulation construction steps, performing full validation only on the final base_sim object at line 540. Additionally, grid size validation was moved from checking every generated simulation in sim_dict to a single check of base_sim before simulation generation, reducing redundant checks.

Key changes:

• Updated _sim_with_sources docstring from "troubleshooting" to "plotting" for clarity
• Replaced deprecated .copy(update=...) method with .updated_copy(**...)
• Added validate=False and deep=False flags to intermediate updated_copy() calls
• Moved grid size validation in sim_dict() to run once on base_sim instead of per-simulation
• Added explicit GridSpec.from_grid() call in base_sim property before final validation

Confidence Score: 4/5

• This PR is safe to merge with minimal risk - focused performance optimization with validation preserved where it matters
• The changes are well-structured optimizations that skip redundant validation steps during intermediate simulation construction while ensuring the final simulation is fully validated. The grid size checks are moved to run once instead of multiple times, which is more efficient. The only minor concern is lack of a CHANGELOG entry for this performance improvement.
• No files require special attention

Important Files Changed

File Analysis

Filename	Score	Overview
tidy3d/plugins/smatrix/component_modelers/terminal.py	4/5	Performance optimization: skip validation during intermediate simulation copies, validate only the final base_sim. Grid size validation moved earlier to check once instead of per-simulation.

Sequence Diagram

sequenceDiagram
    participant User
    participant TCM as TerminalComponentModeler
    participant BaseSim as _base_sim_no_radiation_monitors
    participant FinalSim as base_sim
    participant SimDict as sim_dict
    
    User->>TCM: Create modeler
    TCM->>BaseSim: Build intermediate simulation
    Note over BaseSim: updated_copy(validate=False, deep=False)
    BaseSim-->>TCM: Simulation without validation
    
    TCM->>FinalSim: Add radiation monitors
    Note over FinalSim: GridSpec.from_grid() + validate=True
    FinalSim-->>TCM: Fully validated base_sim
    
    User->>TCM: Access sim_dict property
    TCM->>FinalSim: Check grid size (once)
    Note over FinalSim: _check_grid_size_at_ports()<br/>_check_grid_size_at_wave_ports()
    
    loop For each network_index
        TCM->>SimDict: Create simulation with source
        Note over SimDict: updated_copy(validate=False, deep=False)
        SimDict-->>TCM: Simulation copy (no validation)
    end
    
    TCM-->>User: SimulationMap with all simulations

Loading

[greptile-apps]

_{1 file reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[weiliangjin2021]

Thanks for the efforts! Two main questions:

• Are most time in validation spent in meshing? If so, GridSpec.from_grid should address the issue already, and there is no need to disable validation?
• While GridSpec.from_grid can avoid re-meshing, the original grid_spec information is also lost. Maybe we can dump it in grid_spec.attrs?

[yaugenst-flex]

@momchil-flex mind sneaking in SCRF here?

[Copilot]

Pull Request Overview

This PR optimizes TerminalComponentModeler performance by deferring validation and deep copying until necessary during simulation construction. The changes skip expensive operations during intermediate steps and validate only the final base_sim object. Grid size validation is moved from checking every simulation in sim_dict to a single check of base_sim.

• Adds validate=False and deep=False flags to intermediate updated_copy() calls
• Moves grid size validation from per-simulation to a single check on base_sim
• Replaces deprecated .copy(update=...) with .updated_copy(**...)

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
tidy3d/plugins/smatrix/component_modelers/terminal.py	Performance optimizations: skip validation/deep copying in intermediate simulation construction, validate only final base_sim; move grid size checks to run once
tests/test_plugins/test_array_factor.py	Update test to return modeler instead of sim_unit, with corresponding usage changes; comment out override_structures assertion
.github/workflows/tidy3d-python-client-tests.yml	Add "SCRF" to allowed JIRA project prefixes

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[momchil-flex]

• Are most time in validation spent in meshing? If so, GridSpec.from_grid should address the issue already, and there is no need to disable validation?

• While GridSpec.from_grid can avoid re-meshing, the original grid_spec information is also lost. Maybe we can dump it in grid_spec.attrs?

So a bunch of the skipped validators are in copies that are done before we finally update the grid with the from_grid method. That happens here and is what the comment refers to. I think your question is valid for validations that happen after the base sim construction, like this one when doing the sim dict. My profiling showed that while avoiding the grid construction does speed things up, there can still be quite some overhead in validating especially when there are many ports, like e.g. due to calls to intersections_plane in some validators.

While GridSpec.from_grid can avoid re-meshing, the original grid_spec information is also lost. Maybe we can dump it in grid_spec.attrs?

I guess that's a good point. Normally we try for attrs not to be functional though. So if we want to keep the original grid spec somewhere to potentially use, the better approach is likely to create a proper field for it.

@dmarek-flex @weiliangjin2021 you should help me reason about this? Note that I had to fix one test which was checking for override structures in derived simulations. But now, such derived simulations have CustomGrid, and the original grid_spec is only in the modeler (and that one doesn't contain the override structures that were created due to ports, etc.) I could see how this could be problematic. For example, if I wanted to create a new modeler starting from one of these simulations, I have lost track of the original auto grid, and if I e.g. do .updated_copy on the simulation and change some structures, or its size, etc. the CustomGrid becomes stale.

@dmarek-flex brought up pydantic v2's computed_field yesterday, which can help somewhat with this, in that the grid, which is currently a cached_property, can also be serialized once computed. This could be nice for passing the sim_dict to the server and not having to re-mesh each simulation when it starts running, but it doesn't really avoid the meshing when constructing each of the sim_dict simulations the first time. We also need to be able to set the cached property when constructing, while still ensuring that if anything in the simulation gets updated, that cached property gets cleared. This is probably doable after the pydantic v2 update @yaugenst-flex ?

Practically speaking though, for the current update, it is not clear to me whether we can proceed as proposed here + maybe adding an extra field for the grid spec from which the simulations were derived... or adding it to the attrs, but both seems like not ideal solutions.

[github-actions]

Diff Coverage

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

• tidy3d/plugins/smatrix/component_modelers/terminal.py (100%)

Summary

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

[yaugenst-flex]

We also need to be able to set the cached property when constructing, while still ensuring that if anything in the simulation gets updated, that cached property gets cleared

depending on how important this is we can also do this now, i don't think it's necessarily pydantic v2 related. we have similar cache invalidation mechanisms in place for the autograd traced fields

what about having this in _cached_properties? in principle this can also be set directly, e.g. in a pre validator

[momchil-flex]

We also need to be able to set the cached property when constructing, while still ensuring that if anything in the simulation gets updated, that cached property gets cleared

depending on how important this is we can also do this now, i don't think it's necessarily pydantic v2 related. we have similar cache invalidation mechanisms in place for the autograd traced fields

what about having this in _cached_properties? in principle this can also be set directly, e.g. in a pre validator

Yeah all things considered maybe this is worth a try? What about serializing it too? Do you think it's a relatively straightforward addition?

[yaugenst-flex]

Yeah all things considered maybe this is worth a try? What about serializing it too? Do you think it's a relatively straightforward addition?

it's possible.. for serialization we'd have to add some custom handling in maybe to_hdf5. conceptually it's straightforward.. in terms of design it's not particularly nice so it kind of depends on how important it is and how much it helps 😄

do we have to do this in the basemodel or can we keep this change a bit more localized? i'd probably prefer it if we could keep this mostly contained within the component modeler, but not sure how feasible, i'm not too familiar with the issue

[dmarek-flex]

you should help me reason about this?

TerminalComponentModeler could have a cached_property for the final grid_spec, that can be used to regenerate the grid if needed and also for troubleshooting? I don't see the CustomGrid becoming stale as a problem, it will recreate as soon as you do an updated_copy and try to access the field, right?

Maybe even the grid could be a cached_property of the TerminalComponentModeler, with a helper to recreate the grid.

[momchil-flex]

you should help me reason about this?

TerminalComponentModeler could have a cached_property for the final grid_spec, that can be used to regenerate the grid if needed and also for troubleshooting? I don't see the CustomGrid becoming stale as a problem, it will recreate as soon as you do an updated_copy and try to access the field, right?

Maybe even the grid could be a cached_property of the TerminalComponentModeler, with a helper to recreate the grid.

So the original grid_spec is always stored in modeler.simulation.grid_spec.

But what if you do

modeler = TerminalComponentModeler(...)
base_sim = modeler.base_sim # this now will have a CustomGrid
new_sim = base_sim.updated_copy(structures=..., size=...) # now this grid no longer matches structures
new_modeler = TerminalComponentModeler(simulation=new_sim) # problem propagates

I don't know how much we want to worry about this? Also this reminds me about something else we discussed yesterday: I think modeler quietly assumes that modeler.simulation.grid_spec is an AutoGrid, as a bunch of override structures are added to the base_sim, etc. We probably need to validate that a modeler is created with an AutoGrid? In which case the above workflow will at least error on the fourth line, even though the third one is already problematic.

[dmarek-flex]

I don't know how much we want to worry about this? Also this reminds me about something else we discussed yesterday: I think modeler quietly assumes that modeler.simulation.grid_spec is an AutoGrid, as a bunch of override structures are added to the base_sim, etc. We probably need to validate that a modeler is created with an AutoGrid?

Ok, well I am not worried about it. Your second point is a good one though. I would say we should allow UniformGrid/CustomGrid in the input Simulation but skip creation of mesh overrides and other grid modifications if that happens to be the case.

[momchil-flex]

I don't know how much we want to worry about this? Also this reminds me about something else we discussed yesterday: I think modeler quietly assumes that modeler.simulation.grid_spec is an AutoGrid, as a bunch of override structures are added to the base_sim, etc. We probably need to validate that a modeler is created with an AutoGrid?

Ok, well I am not worried about it. Your second point is a good one though. I would say we should allow UniformGrid/CustomGrid in the input Simulation but skip creation of mesh overrides and other grid modifications if that happens to be the case.

Haha ok then I won't worry about it either. Will you work on the modification for the second point?

To @weiliangjin2021's original question

While GridSpec.from_grid can avoid re-meshing, the original grid_spec information is also lost. Maybe we can dump it in grid_spec.attrs?

Is it sufficient that this remains in modeler.simulation.grid_spec?

[weiliangjin2021]

My profiling showed that while avoiding the grid construction does speed things up, there can still be quite some overhead in validating especially when there are many ports, like e.g. due to calls to intersections_plane in some validators

This seems to be caused by cleanup_shapely_object introduced in this PR. It's slowing down meshing as well, and it has been causing some issues previously. I'll try to improve it or disable it.

pydantic v2's computed_field yesterday, which can help somewhat with this, in that the grid, which is currently a cached_property, can also be serialized once computed

This is a good idea, and it can be useful in many cases if the cached_property can be uploaded so that it doesn't need to be re-computed on the server. But one issue is that if one modifies their tidy3d code locally and generates wrong cached_property, the solver can be dealing with odd simulations.

[momchil-flex]

Cached properties are cleared if the model is updated. Well, our implementation specifically which will remain in v2 I assume @yaugenst-flex ?

[weiliangjin2021]

But what if you do

modeler = TerminalComponentModeler(...)
base_sim = modeler.base_sim # this now will have a CustomGrid
new_sim = base_sim.updated_copy(structures=..., size=...) # now this grid no longer matches structures
new_modeler = TerminalComponentModeler(simulation=new_sim) # problem propagates

For now before pydantic v2 is integrated, maybe we can add a private field _grid_spec to Simulation. If it is None, mesh will be generated; otherwise take the mesh from _grid_spec. In modeler, as @dmarek-flex said, modeler can have a cached_property grid_spec that stores the custom grid. In generating base_sim and sim_dict, we populate both grid_spec and _grid_spec.

[momchil-flex]

For now before pydantic v2 is integrated, maybe we can add a private field _grid_spec to Simulation.

A private field would not be serialized. And in any case adding it as a simple field (rather than something like a cached property) definitely runs the risk that if the user updates the simulation without updating that field, it will be stale (which I guess is the same problem like if we directly set grid_spec to a custom grid). So maybe your point is let's live with this problem, but make sure that we keep the original grid_spec as well in Simulation? But dunno, I kind of don't like having two sources of truth, like if I forget about the "private field" (which will need to be not private) and I modify my grid_spec and I'm wondering why are things not changing, or I don't realize that things won't be.

[dmarek-flex]

Haha ok then I won't worry about it either. Will you work on the modification for the second point?

I started working on this but quickly found that it basically already works. The GridSpec can hold snapping points, mesh overrides even if the grid types along each dimension are Uniform. They are simply ignored.

[momchil-flex]

I think the test that I commented out could be pointing to a problematic use case?

tidy3d/tests/test_plugins/test_array_factor.py

Line 440 in 28a6bb5

# assert len(sim_array.grid_spec.override_structures) == 7

sim_unit is taken from the modeler's sim_dict. Then sim_array is created from that using array_calculator.make_antenna_array. But sim_unit already has CustomGrid, while make_antenna_array seems to be doing something with override structures. I don't really understand the details but is the CustomGrid breaking the assumptions of the array calculator?

Similar comment to @dmarek-flex 's comment above: yes override structures are allowed in UnifromGrid and CustomGrid, so things will not error e.g. when refining around ports, but they will also not actually get refined. Isn't this bad?

[dmarek-flex]

I think the test that I commented out could be pointing to a problematic use case?

My confusion was that from_grid is what strips those fields away, so that makes the issue clearer.

so things will not error e.g. when refining around ports, but they will also not actually get refined. Isn't this bad?

It's probably bad, but I don't want to completely disallow it. I will return to this, and add validator to check if the user has chosen refinement options for ports and whether the grid_spec contains any type of auto grid. Basically if the user really wants custom/uniform grid they need to be extremely explicit.

I don't really understand the details but is the CustomGrid breaking the assumptions of the array calculator?

Array calculator is just duplicating simulations in an array, so I think we just need to change this test to either expect 0 overrides or use the base simulation to ensure mesh overrides are duplicated when present @dbochkov-flexcompute

[momchil-flex]

Array calculator is just duplicating simulations in an array, so I think we just need to change this test to either expect 0 overrides or use the base simulation to ensure mesh overrides are duplicated when present @dbochkov-flexcompute

Yeah I don't really understand those assertions I had to comment out.

I guess if we think this is safe for now though, what I could do is maybe just add the original grid_spec to the attrs of the base_sim as originally suggested by @weiliangjin2021 . We keep a track of it but in principle it shouldn't be expected to be functional. And the modeler always keeps the original one in modeler.simulation.grid_spec.

[weiliangjin2021]

I guess if we think this is safe for now though, what I could do is maybe just add the original grid_spec to the attrs of the base_sim as originally suggested by @weiliangjin2021 . We keep a track of it but in principle it shouldn't be expected to be functional. And the modeler always keeps the original one in modeler.simulation.grid_spec.

Sounds good to me

[momchil-flex]

So I just added this one line, does it look good now @weiliangjin2021 @dmarek-flex ?

[dbochkov-flexcompute]

I don't really understand the details but is the CustomGrid breaking the assumptions of the array calculator?

Array calculator is just duplicating simulations in an array, so I think we just need to change this test to either expect 0 overrides or use the base simulation to ensure mesh overrides are duplicated when present @dbochkov-flexcompute

Yeah, actually when implementing the array calculator I didn't take into account the possibility of CustomGrid. I think it's ok to comment out/remove those lines for now as we are planning to do a major refactor of array calculator anyway in the future

[weiliangjin2021]

All look good!

[momchil-flex]

Closing as changes are incorporated in #2978