feat(tidy3d): FXC-4607-autograd-for-clip-operation #3104
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ### Added | ||
|
|
||
| - Addded autograd support for `ClipOperation` geometries like unions or intersections of geometries. |
There was a problem hiding this comment.
syntax: Typo: 'Addded' should be 'Added'
| - Addded autograd support for `ClipOperation` geometries like unions or intersections of geometries. | |
| - Added autograd support for `ClipOperation` geometries like unions or intersections of geometries. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: CHANGELOG.md
Line: 11:11
Comment:
**syntax:** Typo: 'Addded' should be 'Added'
```suggestion
- Added autograd support for `ClipOperation` geometries like unions or intersections of geometries.
```
How can I resolve this? If you propose a fix, please make it concise.| # "polyslab": ("vertices",), | ||
| "polyslab": ("slab_bounds", 0), | ||
| "mesh": ("mesh_dataset", "surface_mesh"), | ||
| } |
There was a problem hiding this comment.
style: Remove commented code; rely on version control for history
| # "polyslab": ("vertices",), | |
| "polyslab": ("slab_bounds", 0), | |
| "mesh": ("mesh_dataset", "surface_mesh"), | |
| } | |
| FIELD_PATHS = { | |
| "box": ("center", 0), | |
| "polyslab": ("slab_bounds", 0), | |
| "mesh": ("mesh_dataset", "surface_mesh"), | |
| } |
Context Used: Rule from dashboard - Remove commented-out or obsolete code; rely on version control for history. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/test_autograd_clip_operation.py
Line: 392:395
Comment:
**style:** Remove commented code; rely on version control for history
```suggestion
FIELD_PATHS = {
"box": ("center", 0),
"polyslab": ("slab_bounds", 0),
"mesh": ("mesh_dataset", "surface_mesh"),
}
```
**Context Used:** Rule from `dashboard` - Remove commented-out or obsolete code; rely on version control for history. ([source](https://app.greptile.com/review/custom-context?memory=0ee31792-33ef-48e0-8328-48277c2ef438))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| # if geometry_type == "mesh": | ||
| # rtol = 6e-2 | ||
| # atol = 8e-2 | ||
|
|
There was a problem hiding this comment.
style: Remove commented-out code; use version control for history instead
| # if geometry_type == "mesh": | |
| # rtol = 6e-2 | |
| # atol = 8e-2 | |
| rtol = 6e-2 | |
| atol = 3e-2 |
Context Used: Rule from dashboard - Remove commented-out or obsolete code; rely on version control for history. (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/test_autograd_clip_operation.py
Line: 437:440
Comment:
**style:** Remove commented-out code; use version control for history instead
```suggestion
rtol = 6e-2
atol = 3e-2
```
**Context Used:** Rule from `dashboard` - Remove commented-out or obsolete code; rely on version control for history. ([source](https://app.greptile.com/review/custom-context?memory=0ee31792-33ef-48e0-8328-48277c2ef438))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.| # def test_box_mesh_derivative_timing(): | ||
| # box = td.Box(center=(0.1, -0.2, 0.3), size=(1.2, 0.8, 1.6)) | ||
| # | ||
| # derivative_info = DummyDerivativeInfo( | ||
| # constant_grad, | ||
| # paths=[("center",), ("size",)], | ||
| # ) | ||
| # derivative_info.bounds = box.bounds | ||
| # derivative_info.bounds_intersect = box.bounds | ||
| # | ||
| # # timings = _runtime_comparison_by_path(box, derivative_info) | ||
| # | ||
| # for path, (mesh_time, legacy_time, ratio) in timings.items(): | ||
| # print( | ||
| # f"Box | path={path}: " | ||
| # f"mesh {mesh_time * 1e3:.2f} ms, " | ||
| # f"legacy {legacy_time * 1e3:.2f} ms, " | ||
| # f"ratio={ratio:.2f}x", | ||
| # flush=True, | ||
| # ) |
There was a problem hiding this comment.
style: Remove commented-out code before finalizing the PR
Context Used: Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/test_mesh_derivatives.py
Line: 103:122
Comment:
**style:** Remove commented-out code before finalizing the PR
**Context Used:** Rule from `dashboard` - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... ([source](https://app.greptile.com/review/custom-context?memory=f6a669d8-0060-4f11-9cac-10ac7ee749ea))
How can I resolve this? If you propose a fix, please make it concise.| # timings = _runtime_comparison_by_path(polyslab, derivative_info) | ||
| # | ||
| # for path, (mesh_time, legacy_time, ratio) in timings.items(): | ||
| # print( | ||
| # f"Polyslab vertices={len(verts)} | path={path}: " | ||
| # f"mesh {mesh_time * 1e3:.2f} ms, " | ||
| # f"legacy {legacy_time * 1e3:.2f} ms, " | ||
| # f"ratio={ratio:.2f}x", | ||
| # flush=True, | ||
| # ) |
There was a problem hiding this comment.
style: Remove commented-out timing code; use version control for code history
Context Used: Rule from dashboard - Remove commented-out or obsolete code; rely on version control for history. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/test_mesh_derivatives.py
Line: 199:208
Comment:
**style:** Remove commented-out timing code; use version control for code history
**Context Used:** Rule from `dashboard` - Remove commented-out or obsolete code; rely on version control for history. ([source](https://app.greptile.com/review/custom-context?memory=0ee31792-33ef-48e0-8328-48277c2ef438))
How can I resolve this? If you propose a fix, please make it concise.| vjps_geo = geometry._compute_derivatives_via_mesh( | ||
| geo_info, clip_operation=(self, which) | ||
| ) |
There was a problem hiding this comment.
logic: Direct call to geometry._compute_derivatives_via_mesh assumes all geometries implement this method, but the base implementation raises NotImplementedError. This could cause runtime errors for geometry types that haven't implemented mesh-based derivatives. How should the system handle geometries that don't yet support mesh-based derivatives in clip operations?
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/geometry/base.py
Line: 3622:3624
Comment:
**logic:** Direct call to `geometry._compute_derivatives_via_mesh` assumes all geometries implement this method, but the base implementation raises `NotImplementedError`. This could cause runtime errors for geometry types that haven't implemented mesh-based derivatives. How should the system handle geometries that don't yet support mesh-based derivatives in clip operations?
How can I resolve this? If you propose a fix, please make it concise.| # return td.TriangleMesh.from_vertices_faces(vertices, faces) | ||
| mesh = td.TriangleMesh.from_triangles(vertices[faces]) | ||
| # mesh_fix = mesh.fix_winding() | ||
| # tr1 = mesh.mesh_dataset.surface_mesh.values | ||
| # tr2 = mesh_fix.mesh_dataset.surface_mesh.values | ||
| # print(tr1 == tr2) | ||
| # exit() | ||
| return mesh |
There was a problem hiding this comment.
style: Remove commented-out debugging code and dead code paths before finalizing.
| # return td.TriangleMesh.from_vertices_faces(vertices, faces) | |
| mesh = td.TriangleMesh.from_triangles(vertices[faces]) | |
| # mesh_fix = mesh.fix_winding() | |
| # tr1 = mesh.mesh_dataset.surface_mesh.values | |
| # tr2 = mesh_fix.mesh_dataset.surface_mesh.values | |
| # print(tr1 == tr2) | |
| # exit() | |
| return mesh | |
| mesh = td.TriangleMesh.from_triangles(vertices[faces]) | |
| return mesh |
Context Used: Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/numerical/test_autograd_clip_operation_numerical.py
Line: 135:142
Comment:
**style:** Remove commented-out debugging code and dead code paths before finalizing.
```suggestion
mesh = td.TriangleMesh.from_triangles(vertices[faces])
return mesh
```
**Context Used:** Rule from `dashboard` - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... ([source](https://app.greptile.com/review/custom-context?memory=f6a669d8-0060-4f11-9cac-10ac7ee749ea))
How can I resolve this? If you propose a fix, please make it concise.| if draft_mode: | ||
| web.upload(sim, task_name="draft") | ||
| exit() |
There was a problem hiding this comment.
logic: Remove draft_mode debugging code with exit() call - this will cause test failures.
| if draft_mode: | |
| web.upload(sim, task_name="draft") | |
| exit() | |
| if draft_mode: | |
| web.upload(sim, task_name="draft") | |
| return [0.0] # Return dummy value for draft mode |
Context Used: Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/numerical/test_autograd_clip_operation_numerical.py
Line: 258:260
Comment:
**logic:** Remove `draft_mode` debugging code with `exit()` call - this will cause test failures.
```suggestion
if draft_mode:
web.upload(sim, task_name="draft")
return [0.0] # Return dummy value for draft mode
```
**Context Used:** Rule from `dashboard` - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... ([source](https://app.greptile.com/review/custom-context?memory=f6a669d8-0060-4f11-9cac-10ac7ee749ea))
How can I resolve this? If you propose a fix, please make it concise.| ) | ||
| params0 = anp.array(BASE_OFFSET, dtype=float) | ||
| _, grad = value_and_grad(adjoint_objective)(params0) | ||
| # return |
There was a problem hiding this comment.
style: Remove commented early return statement used for debugging.
Context Used: Rule from dashboard - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/numerical/test_autograd_clip_operation_numerical.py
Line: 369:369
Comment:
**style:** Remove commented early return statement used for debugging.
**Context Used:** Rule from `dashboard` - Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... ([source](https://app.greptile.com/review/custom-context?memory=f6a669d8-0060-4f11-9cac-10ac7ee749ea))
How can I resolve this? If you propose a fix, please make it concise.| autograd_grad = np.asarray(grad, dtype=float).ravel() | ||
| fd_grad = _finite_difference_gradient(fd_objective, params0, FINITE_DIFF_STEP) | ||
| angle = angled_overlap_deg(autograd_grad, fd_grad) | ||
| assert angle < 13, ( |
There was a problem hiding this comment.
style: Replace magic number 13 with a named constant or use ANGLE_OVERLAP_FD_ADJ_THRESH_DEG + 1 to make the threshold relationship explicit.
Context Used: Rule from dashboard - Avoid hardcoding values ("magic numbers") that can be programmatically derived from data; use named ... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/test_components/autograd/numerical/test_autograd_clip_operation_numerical.py
Line: 424:424
Comment:
**style:** Replace magic number `13` with a named constant or use `ANGLE_OVERLAP_FD_ADJ_THRESH_DEG + 1` to make the threshold relationship explicit.
**Context Used:** Rule from `dashboard` - Avoid hardcoding values ("magic numbers") that can be programmatically derived from data; use named ... ([source](https://app.greptile.com/review/custom-context?memory=6661caa7-319d-4caf-8111-723b32818d62))
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Have to wait for sphere to be merged, will add some tests and special handling for it then.
Greptile Summary
ClipOperationgeometries like unions, intersections, and differences to enable gradient-based optimization workflowsBox,Cylinder,PolySlab,TriangleMesh) for handling complex surface interactions during clipped operationsImportant Files Changed
Confidence score: 3/5
Context used (5)
dashboard- Remove commented-out or obsolete code; rely on version control for history. (source)dashboard- Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)dashboard- Update the CHANGELOG.md file when making changes that affect user-facing functionality or fix bugs. (source)dashboard- Use changelog categories correctly: "Fixed" for bug fixes, "Changed" for modifications to existing f... (source)dashboard- Assert the direct outcome of an operation rather than a side effect (like a log message) when possib... (source)