-
Notifications
You must be signed in to change notification settings - Fork 67
fix(tidy3d): FXC-4563-use-2-d-intersections-for-adjoint-monitor-sizes-in-2-d-simulations #3096
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 1 comment
4c5b691 to
2ecf314
Compare
2ecf314 to
b1d5083
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 2 comments
1e0bc03 to
481b3fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, no comments
…-in-2-d-simulations
481b3fa to
bd8e561
Compare
| """make a random DataArray out of supplied coordinates and data_type.""" | ||
| data_shape = [len(coords[k]) for k in data_array_type._dims] | ||
| np.random.seed(1) | ||
| np.random.seed(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a funny one. It caused a gradient being 2e-10 by chance in test_multi_frequency_equivalence
| index_to_keys[index].append(fields) | ||
|
|
||
| freqs = self._freqs_adjoint | ||
| sim_plane = self if self.size.count(0.0) == 1 else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we have adjoint monitors that are 3D, but the simulation is 2D, I believe the clipping will happen naturally in that the adjoint monitor will just have a 2D overlap with the simulation and so we will only get 2D data. I might be missing something though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it does and this is fine most of the times. I target cases like here where the geometry bounds do not reflect the bounds within the sim plane: The non-centered Sphere has smaller bounds in the simulation plane. Same would apply for non-prism-like meshes.
Basically it is more about performance/efficiency as we may only shrink the monitor potentially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that makes sense!
Previously, we just took the geometry bounds to construct adjoint monitors. This PR uses the plane intersection in 2D simulations.
Greptile Overview
Greptile Summary
This PR improves adjoint monitor sizing in 2D simulations by using planar geometry intersections instead of full bounding boxes. In 2D simulations (detected via
self.size.count(0.0) == 1), the code now callsgeometry.intersections_plane()to compute tight monitor bounds based on the actual geometry cross-section at the simulation plane.Key changes:
simulation.py:4887detects 2D simulations and passes plane info to structure monitor generationstructure.py:328-352implements_box_from_plane_intersection()that computes monitor bounds from plane intersections with multi-level fallback (geometry → bounding box → full bounding box)The implementation handles edge cases through a cascading fallback strategy and maintains backward compatibility for 3D simulations.
Confidence Score: 5/5
Important Files Changed
File Analysis
Sequence Diagram
sequenceDiagram participant Sim as Simulation participant Struct as Structure participant Geom as Geometry participant BoxCls as Box Note over Sim: User calls _make_adjoint_monitors Sim->>Sim: Check if 2D via size.count(0.0) == 1 alt Is 2D simulation Sim->>Sim: sim_plane = self else Is 3D simulation Sim->>Sim: sim_plane = None end loop For each structure Sim->>Struct: _make_adjoint_monitors(freqs, index, field_keys, plane) alt plane is not None (2D case) Struct->>Struct: Call _box_from_plane_intersection Struct->>Geom: geometry.intersections_plane Geom-->>Struct: Return intersection shapes alt Intersections found Struct->>Struct: Compute union of bounds else No intersections Struct->>Geom: geom_box.intersections_plane alt Box intersections found Struct->>Struct: Compute union of bounds else Still no intersections Struct->>Struct: Use geom_box as fallback end end Struct->>BoxCls: Box.from_bounds(rmin, rmax) BoxCls-->>Struct: Return planar box else plane is None (3D case) Struct->>Struct: box = geom_box end Struct->>Struct: Create FieldMonitor with box Struct->>Struct: Create PermittivityMonitor with box Struct-->>Sim: Return monitors end Sim-->>Sim: Collect all monitors