XY transposable sim.plot() plots (issue1072) VERSION 2 #2544
Conversation
|
I don't want to add complexity to your code where it's not needed, @tylerflex ! The
|
|
This code has not yet been tested. It probably won't even run. I am beginning the (manual) testing process now. |
tidy3d/components/geometry/base.py
Outdated
| @@ -1315,6 +1345,7 @@ def to_gdstk( | |||
| z: Optional[float] = None, | |||
There was a problem hiding this comment.
we probably dont need to pass transpose to these gds functions, these are used to export the polygons to file.
|
hey @jewettaijfc , this is an interesting approach. I think it looks promising! I would have a few pieces of advice at this stage:
Thanks for the effort on this! good luck on the testing. |
For sure. Regarding my work-flow, I don't put PRs up for review until I have some unit tests. It takes a little extra time to write them, but unit tests have saved my butt so many times.
Thanks! I'll fix that now.
I will let you decide. But I suggest we use the same name for all functions. (I like both
I was wondering about this too. I will look into that.
Thank you! |
|
Currently:
Here are some things fixed in this PR (which weren't working before in PR2537):
Here are some things that are currently broken in this PR (which were working in PN2537):
|
jewettaijfc
changed the title
NOTE: I left some print() statements in the code that I'm still using for (manual) debugging. I'll remove these soon. |
|
|
I'm done with manual testing. I started writing unit tests. |
|
Oops. I guess I wasn't done with manual testing.
|
|
|
| @@ -89,7 +89,6 @@ class PolySlab(base.Planar): | |||
| @staticmethod | |||
| def make_shapely_polygon(vertices: ArrayLike) -> shapely.Polygon: | |||
| """Make a shapely polygon from some vertices, first ensures they are untraced.""" | |||
| vertices = get_static(vertices) | |||
There was a problem hiding this comment.
I think this is causing the error, this line should be put back
There was a problem hiding this comment.
yup! just verified that it works after adding it back
There was a problem hiding this comment.
Wow. Thank you! This would have taken me hours to figure out.
| return [self.make_shapely_polygon(vertices_z)] | ||
| vertices = vertices_z | ||
| if swap_axes: | ||
| vertices = vertices[:, (1, 0)] # swap column 0 (x coords) with column 1 (y coords) |
There was a problem hiding this comment.
I'm worried this code will not work with autograd (the package I mentioned that handles our automatic differentiation).
I would suggest replacing with
vertices = vertices[:, ::-1] # reverse the second axisThere was a problem hiding this comment.
Thanks!
Other than pop_axis_and_swap() and unpop_axis_and_swap(), this is the only other place in the code (that I am aware of) containing non-trivial modifications. I need to write a unit-test for it.
| max_bounds = list(field_data_bounds[1]) | ||
| min_bounds.pop(axis) | ||
| max_bounds.pop(axis) | ||
| _, min_bounds = self.pop_axis_and_swap(field_data_bounds[0], axis, swap_axes=swap_axes) |
There was a problem hiding this comment.
good catch, this is probably more robust than what we had before.
jewettaijfc
force-pushed
the
jewettaijfc/issue1072_v2
branch
from
2f38175 to
de2d14d
Compare
|
I think EDIT 2025-6-10: I JUST REMOVED THOSE COMMITS FROM THIS PR. THEY NO LONGER APPEAR IN THE HISTORY In retrospect, I'm not sure I should be modifying these functions. Most of the
|
|
It sounds like |
jewettaijfc
force-pushed
the
jewettaijfc/issue1072_v2
branch
from
2510f60 to
f0995ca
Compare
|
Minor issue: A few days ago, commit Note to self: Later on, if we really want to purge that file from the history, we can use: Note: If you are not in the main branch, you probably have to use this command instead: Clean up: I am not brave enough to try this yet. |
|
what I would recommend is doing |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implements axis transposition functionality across Tidy3D's visualization system by adding a transpose parameter to plot-related functions.
- Core implementation centers on new
pop_axis_and_swap()andunpop_axis_and_swap()functions in geometry/base.py that handle coordinate transformations - Systematically propagates
transposeparameter through all plotting functions that accept axis arguments (x,y,z) - Notable update to
intersections_tilted_plane()in geometry/base.py contains potential bug where X,Y axis order ignores normal direction - Comprehensive test coverage added across components including parameterized tests for both
transpose=True/Falsecases - Clean implementation using strategy of swapping geometry/limits pre-matplotlib rather than post-rendering axis manipulation
25 files reviewed, 3 comments
Edit PR Review Bot Settings | Greptile
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/geometry/base.pyLines 2199-2207 2199 if section is None:
2200 return []
2201 path, _ = section.to_planar(to_2D=to_2D)
2202 if transpose:
! 2203 path.vertices = path.vertices[:, ::-1] # swap column 0 with column 1
2204
2205 print(f"Box._do_intersections_tilted_plane({transpose=}), {path=}") # DEBUG
2206
2207 return path.polygons_fullLines 3127-3136 3127 List of 2D shapes that intersect plane.
3128 For more details refer to
3129 `Shapely's Documentation <https://shapely.readthedocs.io/en/stable/project.html>`_.
3130 """
! 3131 a = self.geometry_a.intersections_tilted_plane(normal, origin, to_2D, transpose=transpose)
! 3132 b = self.geometry_b.intersections_tilted_plane(normal, origin, to_2D, transpose=transpose)
3133 geom_a = shapely.unary_union([Geometry.evaluate_inf_shape(g) for g in a])
3134 geom_b = shapely.unary_union([Geometry.evaluate_inf_shape(g) for g in b])
3135 return ClipOperation.to_polygon_list(self._shapely_operation(geom_a, geom_b))tidy3d/components/geometry/mesh.pyLines 554-564 554 section = self.trimesh.section(plane_origin=origin, plane_normal=normal)
555 if section is None:
556 return []
557 path, _ = section.to_planar(to_2D=to_2D)
! 558 if transpose:
! 559 path.vertices = path.vertices[:, ::-1] # swap column 0 (X) with column 1 (Y)
! 560 print(f"TriangleMesh.intersections_tilted_plane({transpose=}), {path=}") # DEBUG
561 return path.polygons_full
562
563 def intersections_plane(
564 self,Lines 634-642 634 "Unable to compute 'TriangleMesh.intersections_plane'. "
635 "Using bounding box instead."
636 )
637 log.warning(f"Error encountered: {e}")
! 638 return self.bounding_box.intersections_plane(x=x, y=y, z=z, transpose=transpose)
639
640 def inside(
641 self, x: np.ndarray[float], y: np.ndarray[float], z: np.ndarray[float]
642 ) -> np.ndarray[bool]:tidy3d/components/geometry/polyslab.pyLines 631-639 631 if section is None:
632 return []
633 path, _ = section.to_planar(to_2D=to_2D)
634 if transpose:
! 635 path.vertices = path.vertices[:, ::-1] # swap column 0 (X) with column 1 (Y)
636 print(f"TriangleMesh.intersections_tilted_plane({transpose=}), {path=}") # DEBUG
637 return path.polygons_full
638
639 def _intersections_normal(self, z: float, transpose: bool = False):Lines 663-671 663 dist = -z_local * self._tanq
664 vertices_z = self._shift_vertices(self.middle_polygon, dist)[0]
665 vertices = vertices_z
666 if transpose:
! 667 vertices = vertices[:, ::-1] # swap column 0 (x coords) with column 1 (y coords)
668 return [self.make_shapely_polygon(vertices)]
669
670 def _intersections_side(self, position, axis, transpose: bool = False) -> list:
671 """Find shapely geometries intersecting planar geometry with axis orthogonal to slab.tidy3d/components/geometry/primitives.pyLines 113-121 113 angles = np.linspace(0, 2 * np.pi, _N_SHAPELY_QUAD_SEGS * 4 + 1)[:-1]
114 circ = center + np.outer(np.cos(angles), radius * u) + np.outer(np.sin(angles), radius * v)
115 vertices = np.dot(np.hstack((circ, np.ones((angles.size, 1)))), to_2D.T)
116 if transpose:
! 117 vertices = vertices[:, ::-1] # swap column 0 (X) with column 1 (Y)
118 return [shapely.Polygon(vertices[:, :2])]
119
120 def intersections_plane(
121 self,Lines 480-489 480 section = mesh.section(plane_origin=origin, plane_normal=normal)
481 if section is None:
482 return []
483 path, _ = section.to_planar(to_2D=to_2D)
! 484 if transpose:
! 485 path.vertices = path.vertices[:, ::-1] # swap column 0 (X) with column 1 (Y)
486 return path.polygons_full
487
488 def _intersections_normal(self, z: float, transpose: bool = False):
489 """Find shapely geometries intersecting cylindrical geometry with axis normal to slab.tidy3d/components/scene.pyLines 1231-1242 1231 rmin.insert(normal_axis_ind, normal_position)
1232 rmax.insert(normal_axis_ind, normal_position)
1233
1234 if grid is None:
! 1235 if transpose:
! 1236 import warnings
1237
! 1238 warnings.warn(
1239 "UNTESTED! The plotting function you are using has not\n"
1240 "yet been rigorously tested with `transpose=True`.\n"
1241 "To confirm that the plot you are seeing now is correct,\n"
1242 "try again with `transpose=False` and compare the two plots.",Lines 1242-1250 1242 "try again with `transpose=False` and compare the two plots.",
1243 stacklevel=2,
1244 )
1245
! 1246 _, plane_axes_inds = Box.pop_axis_and_swap(
1247 [0, 1, 2], axis=normal_axis_ind, transpose=transpose
1248 )
1249
1250 eps_diag = medium.eps_dataarray_freq(frequency=freq)Lines 2019-2027 2019 plt_type accepts ["doping", "N_a", "N_d"]
2020 """
2021 coords = "xyz"
2022 normal_axis_ind, normal_position = Box.parse_xyz_kwargs(x=x, y=y, z=z)
! 2023 normal_axis, plane_axes = Box.pop_axis_and_swap(
2024 coords, normal_axis_ind, transpose=transpose
2025 )
2026
2027 # make grid for eps interpolationLines 2034-2042 2034 rmin.insert(normal_axis_ind, normal_position)
2035 rmax.insert(normal_axis_ind, normal_position)
2036
2037 # for the time being let's assume we'll always need to generate a mesh
! 2038 _, plane_axes_inds = Box.pop_axis_and_swap(
2039 [0, 1, 2], axis=normal_axis_ind, transpose=transpose
2040 )
2041
2042 # build gridLines 2040-2054 2040 )
2041
2042 # build grid
2043 N = 100
! 2044 coords_2D_mesh = [np.linspace(rmin[d], rmax[d], N) for d in plane_axes_inds]
! 2045 X, Y = np.meshgrid(coords_2D_mesh[0], coords_2D_mesh[1], indexing="ij")
! 2046 coords_2D_lookup = coords_2D_mesh.copy()
! 2047 if transpose:
2048 # `X`, `Y`` are the coordinates that will be displayed to the user. We want to
2049 # change them so they are displayed at new locations with the axes swapped.
! 2050 X, Y = Y, X
2051 # The `coords_2D_lookup[]` arrays will be used to lookup the property
2052 # we are plotting as a function of the real (physical) coordinates.
2053 # This should not change. However the `coords_2D_mesh[]` array's
2054 # order was reversed when we previously invoked `pop_axis_and_swap()`Lines 2052-2060 2052 # we are plotting as a function of the real (physical) coordinates.
2053 # This should not change. However the `coords_2D_mesh[]` array's
2054 # order was reversed when we previously invoked `pop_axis_and_swap()`
2055 # with `transpose=True`. We need to undo that now.
! 2056 coords_2D_lookup.reverse() # = [coords_2D_mesh[1], coords_2D_mesh[0]]
2057
2058 struct_doping = [
2059 np.zeros(X.shape), # let's use 0 for N_a
2060 np.zeros(X.shape), # and 1 for N_dLines 2064-2072 2064 for n, doping in enumerate([electric_spec.N_a, electric_spec.N_d]):
2065 if isinstance(doping, float):
2066 struct_doping[n] = struct_doping[n] + doping
2067 if isinstance(doping, SpatialDataArray):
! 2068 struct_coords = {
2069 "xyz"[d]: coords_2D_lookup[i] for i, d in enumerate(plane_axes_inds)
2070 }
2071 data_2D = doping
2072 # check whether the provided doping data is 2 or 3Dtidy3d/plugins/waveguide/rectangular_dielectric.pyLines 1070-1081 1070 -------
1071 matplotlib.axes._subplots.Axes
1072 The supplied or created matplotlib axes.
1073 """
! 1074 if transpose:
! 1075 import warnings
1076
! 1077 warnings.warn(
1078 "UNTESTED! The plot_geometry_edges() function has not yet been tested\n"
1079 "with `transpose=True`. To confirm that the plot you are seeing now\n"
1080 "is correct, try again with `transpose=False` and compare the two plots.",
1081 stacklevel=2,Lines 1117-1126 1117 ):
1118 p = self._translate(x, y, 0)
1119 plot_x.append(p[u])
1120 plot_y.append(p[v])
! 1121 if transpose:
! 1122 plot_x, plot_y = plot_y, plot_x
1123 ax.plot(plot_x, plot_y, linestyle="-", **kwargs)
1124 ax.set_aspect("equal")
1125 return ax |
|
Hi Tyler. I'm going to bed. |
There was a problem hiding this comment.
Looks pretty good! Just minor comments for now. You can focus on clearing these up + those from the new automated review bot 😆 and then request me again for another review.
Let's have @yaugenst-flex also take a look when he's back.
One thing I think we should do also before merging is pick a handful of existing notebooks at random (maybe 5-10) and just throw a transpose=True in to test this a bit more in some real world situations.
Thanks @jewettaijfc !
| b = td.Box(size=(1, 1, 1)) | ||
| for axis in range(3): | ||
| coords = (1, 2, 3) | ||
| Lz, (Lx, Ly) = b.pop_axis_and_swap(coords, axis=axis, transpose=transpose) |
There was a problem hiding this comment.
just FYI, since this is a static method, you can just call it without initializing the class, eg td.Box.pop_axis_and_swap
tidy3d/components/geometry/base.py
Outdated
| @@ -732,6 +763,7 @@ def pop_axis(coord: tuple[Any, Any, Any], axis: int) -> tuple[Any, tuple[Any, An | |||
| axis : int | |||
| Integer index into 'xyz' (0,1,2). | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
might need to remove this extra line
tidy3d/components/geometry/base.py
Outdated
| transpose: bool = False, | ||
| ) -> tuple[Any, tuple[Any, Any]]: | ||
| """ | ||
| pop_axis_and_swap() is identical to pop_axis(), except that accepts an |
There was a problem hiding this comment.
for any code in docstrings, we need to follow the style guide, otherwise it will show up in plaintext in the docs, please read through this guide as it will come up all of the time as you work on more PRs: https://www.notion.so/flexcompute/Python-Style-Guide-afac5dbbf949463e89c18fe39b7453c9
tidy3d/components/geometry/base.py
Outdated
| axis : int | ||
| Integer index into 'xyz' (0,1,2). | ||
| transpose: bool = False | ||
| Optional: Swap the order of the data from the two remaining axes in the output tuple? |
There was a problem hiding this comment.
check this, at least remove "?" :)
tidy3d/components/geometry/base.py
Outdated
| transpose: bool = False, | ||
| ) -> tuple[Any, Any, Any]: | ||
| """ | ||
| unpop_axis_and_swap() is identical to unpop_axis(), except that accepts |
There was a problem hiding this comment.
same comments as above docstring, check here too.
tidy3d/components/geometry/base.py
Outdated
| @@ -1570,7 +1679,7 @@ def intersections_tilted_plane( | |||
| axis = np.argmax(np.abs(normal)).item() | |||
| coord = "xyz"[axis] | |||
| kwargs = {coord: origin[axis]} | |||
| section = self.intersections_plane(**kwargs) | |||
| section = self.intersections_plane(**kwargs) # <-- BUG?: X,Y axis order ignores normal axis direction +/- | |||
There was a problem hiding this comment.
hm, what is this? could you explain?
There was a problem hiding this comment.
Summary
This is a somewhat outdated comment. But perhaps it's worth looking at this code anyway.
Details (feel free to skip)
I added the comment because the first time I saw the intersections_tilted_plane() function, I attempted to add the transpose: bool = False argument to this function. But I eventually got the impression that this function is not used by the various plot functions, so I removed the transpose argument. But I forgot to remove that comment. But there is still an underlying issue I'm worried about.
Source of concern
-
The
intersections_tilted_plane()figures out which objects intersect the plane determined bynormalandorigin. -
I think it also generates 2D polygon silhouettes of these objects at the location where the plane slices through them. (Please correct me if I'm wrong.)
-
If so, then the coordinates of the polygons in the plane depends on the choice of coordinate system in the plane (i.e. the planar axes basis vectors).
-
I'm not sure how
intersections_tilted_plane()chooses these vectors, but I'm guessing that it normally tries to use a right-handed coordinate system. That means, if$\vec{z'}$ denotes the direction normal to the plane, thenintersections_tilted_plane()normally chooses other two axes in the plane ($\vec{x'}$ and$\vec{y'}$ ) to satisfy:$\vec{x'} \times \vec{y'} = \vec{z'}$ .
As a consequence of this, an object shaped like the letter N might get sliced in a way that rotates it to resemble a letter Z, but not to it's mirror-image (ᴎ). -
However that's no longer guaranteed if
intersections_tilted_plane()usespop_axis(). Thepop_axis("xyz", axis=1)function returns("y", ("x", "z")), and$\vec{x} \times \vec{z} = -\vec{y}$ . That could cause a mirror-image (ᴎ). -
Is this inconsistency a problem? (Maybe not, but I wanted to run this by you anyway.)
-
If there's any doubt, the code should behave correctly if we remove that if statement. (
if np.sum(np.isclose(normal, 0.0)) == 2:). (That if statement was only added to make the code run faster in some situations.)
There was a problem hiding this comment.
I'm actually not sure about this.. if it's a problem we haven't encountered it yet. It would be @weiliangjin2021 who would know but he's on parental leave at the moment. for now maybe we can leave the comment in, but you can do it as
... # NOTE: messagewe tend to use "NOTE" to mark something to come back to later.
There was a problem hiding this comment.
Oops. Sorry. I'll take it out.
(I'll put a notice on my google-calendar to remind me to ask weiliangjin2021 about this.)
… docstring for , as suggested by the "greptile-apps" bot.
…led attempts at auto-linting and subsequent commands erased all that progress.)
jewettaijfc
force-pushed
the
jewettaijfc/issue1072_v2
branch
from
8e2434a to
f9b57ba
Compare
|
(Incidentally, that long list of additional commits was inadvertently generated when I attempted to rebase on top of the latest version of the |
|
Update: The code does not quite work yet. I am going through all of the notebooks in the
(NOTE: Even after I have fixed these problems, I still have many more notebook examples to test. I've only tested about 40% of the notebook examples from tidy3d-notebooks so far. It looks like this PR needs more work on my part) |
|
I put about 6 hours into fixing those functions (mentioned in the previous comment). Those functions are more complicated than anything I have seen earlier. I think I can get them working in a few more hours, but I'm going to bed first. |
|
@jewettaijfc thanks for the efforts on this! If the changes are too complicated, I think it would be fine to discuss just disabling the |
Hi Tyler! I think I can get it working by the end of lunch time. I'd rather have these functions updated as consistently as possible |
|
I think I fixed the problem. (After I clean out all of the extra |
…ed all of the print() statements used for debugging yet
|
I just realized that I also need to fix |
…messages when the new code is (visually) untested.
…or debugging. After I'm sure the code is working, I will remove the rest.
Arrow scalingThe only problem I've detected so far is that arrows are sometimes not scaled correctly when Here's an example. The plot on the right uses I will take a look at this issue, but I want to go through more notebook examples first. |
This was really annoying me today. It looks like this is a preexisting issue with tidy3d plots, and not specific to features I added in this PR. I attempted a quick fix for that issue with a different PR. Now I can continue testing this PR with peace of mind. |
…emented for all the other geometric primitives. Things are in a broken state right now.
|
FYI. This PR is in a broken state right now. I know why. I'm working on it. |
…ed_plane() or _do_intersections_tilted_plane(). This fixed some plots and broke others. After this commit, rotated Box objects work with "transpose=True", and cylinders no longer work with "transpose=True".
…ections_tilted_plane()
…n to accept a argument. This seems to work! More importantly, it saves me from having to dig into the details of how to calculate plane intersections with each type of geometric primitive. I will continue testing. If all goes well, I will clean out the transpose argument from places where it is no longer needed before I send this PR back for review again.
|
This PR addresses he feature request from issue #1072 (adding the ability to swap the horizontal and vertical axes of a simulation plot).
(Note: The
plot_field()andTriangularGridDataset.plot()functions are beyond the scope of this PR.)How to test this PR
transpose=Trueare flipped with respect to the plots that omit thetransposeargument.(I confess I was a little bit sloppy creating this .ipynb file, so I might have forgotten to use
transpose=Trueconsistently in all of the plots. In that case, feel free to addtranspose=Trueto any of the.plotfunctions you see in this file.)Motivation
I can think of 2 different strategies to transpose XY axes:
This PR uses the second strategy.
Specifically:
transposeargument to all plot-related functions that previously acceptedaxisorx,y,zarguments. (Conceptually, these arguments all go together: they specify how to display a plane from a 3D space on the screen.)pop_axis()andunpop_axis(), and the (plot-related) functions that invoke them ...all the way up to the top-levelplot()functions.transpose=Falseeverywhere. So unless someone overrides this by settingtranspose=True, the code should behave as it did before.It seems like I made a lot of complicated changes in this PR, but I really didn't. The only non-trivial changes to the code happen inside
pop_axis()andunpop_axis()(see below). All of the higher-level functions merely propagate thetransposeargument downwards until it is finally processed by eitherpop_axis()orunpop_axis(). This reduces the cognitive burden on the programmer: In each function, I no longer have to think hard about how to deal with the newtransposeargument. I just forward thetransposeargument downwards to any functions that ultimately invoke eitherpop_axis()orunpop_axis(). If we do this consistently, we hopefully shouldn't see surprising behavior later.(UPDATE: These functions are now called
pop_axis_and_swap()andunpop_axis_and_swap().)For completeness, here are the new definitions of
pop_axis_and_swap()andunpop_axis_and_swap():That's all this PR does.