XY transposable sim.plot() plots (issue1072) VERSION 2

[jewettaijfc]

This PR addresses he feature request from issue #1072 (adding the ability to swap the horizontal and vertical axes of a simulation plot).

This PR is an alternative to PR#2537

I don't know which approach will work better in the long run. I will test both.

---

Motivation

I can think of 2 different strategies to transpose XY axes:

1. Wait until the very end, after the matplotlib axes object has been fully defined. Then swap the horizontal and vertical axes at display time.
2. Swap the geometry, limits, and axes-label information before sending them to matplotlib.

• The previous PR #2537 uses a combination of both strategies. It also tries to minimalize change to the existing code needed just to get Simulation.plot() to work.
• This PR uses the second strategy, and it updates all of the plot-related functions, not just Simulation.plot().

Specifically:

• I systematically added a transpose argument to all plot-related functions that previously accepted axis or x, y, z arguments. (Conceptually, these arguments all go together: they specify how to display a plane from a 3D space on the screen.)
• This includes pop_axis() and unpop_axis(), and the (plot-related) functions that invoke them ...all the way up to the top-level plot() functions.
• By default transpose=False everywhere. So unless someone overrides this by setting transpose=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() and unpop_axis() (see below). All of the higher-level functions merely propagate the transpose argument downwards until it is finally processed by either pop_axis() or unpop_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 new transpose argument. I just forward the transpose argument downwards to any functions that ultimately invoke either pop_axis() or unpop_axis(). If we do this consistently, we hopefully shouldn't see surprising behavior later.

(UPDATE: These functions are now called pop_axis_and_swap() and unpop_axis_and_swap().)

---

For completeness, here are the new definitions of pop_axis_and_swap() and unpop_axis_and_swap():

    def pop_axis_and_swap(
        coord: tuple[Any, Any, Any],
        axis: int,
        transpose: bool = False,                         # <-- newly added
    ) -> tuple[Any, tuple[Any, Any]]:
        plane_vals = list(coord)
        axis_val = plane_vals.pop(axis)
        if transpose:                                    # <-- newly added
            plane_vals = [plane_vals[1], plane_vals[0]]  # <-- newly added
        return axis_val, tuple(plane_vals)

    def unpop_axis_and_swap(
        ax_coord: Any,
        plane_coords: tuple[Any, Any],
        axis: int,
        transpose: bool = False,              # <-- newly added
    ) -> tuple[Any, Any, Any]:
	coords = list(plane_coords)
        if transpose:                         # <-- newly added
            coords = [coords[1], coords[0]]   # <-- newly added
        coords.insert(axis, ax_coord)
        return tuple(coords)

That's all this PR does.

[jewettaijfc]

I don't want to add complexity to your code where it's not needed, @tylerflex ! The pop_axis() and unpop_axis() functions appear in many places. Let me know if I made too many modifications. I tried to avoid changing functions which are clearly not plot-related, even if they invoke pop_axis() or unpop_axis(). Specifically:

• I did not modify beam.py, boundary.py, field_projection.py, geometry/polyslab.py, geometry/utils2d.py, grid/*.py, lumped_element.py, medium.py, microwave/formulas/circuit_parameters.py, mode_solver.py, source/field.py, or any files outside of the components/ folder.
• I did not modify any functions that compute derivatives.
• I did not modify geometry/primitives.py, except for the various *intersections*() and *cross_section() functions (which are frequently needed for plotting).
• I did not modify monitor.py, except for the plot related functions.

[jewettaijfc]

This code has not yet been tested. It probably won't even run. I am beginning the (manual) testing process now.

[tylerflex]

we probably dont need to pass transpose to these gds functions, these are used to export the polygons to file.

[tylerflex]

hey @jewettaijfc , this is an interesting approach. I think it looks promising! I would have a few pieces of advice at this stage:

1. invest in a good solid test case. Dig into the test_viz.py and write some plot tests with transpose=True. Learn how to run the unit tests (should be in the developer guide) and that will save you a lot of time as you test this.
2. No need to pass transpose to any functions involving gds, eg to_gds. These are used when importing and exporting to GDSII files. https://www.flexcompute.com/tidy3d/examples/notebooks/GDSImport/
3. Pretty minor comment but might as well start thinking about the argument naming. I am pretty confident that a bool is the right data type for this option, but the naming I'm not sure about.
   a. in the plotting functions, we could also consider like swap_axes : bool = False?
   b. in the pop_axis and unpop_axis, maybe we consider something similar? or swap_planar_coords?
4. The only thing I'm still not sure about / hesitant about is whether we should be modifying these pop_axis and unpop_axis functions directly. I'm leaning towards thinking it's ok, but another way we could approach this to introduce new functions pop_axis_and_swap(coords, axis, swap_axes: bool) that simply calls pop_axis(coords, axis) and swaps the results if swap_axes is true.). Something to think about, but not sure if it improves anything or just complicates things.

Thanks for the effort on this! good luck on the testing.

[jewettaijfc]

hey @jewettaijfc , this is an interesting approach. I think it looks promising! I would have a few pieces of advice at this stage:

1. invest in a good solid test case. Dig into the test_viz.py and write some plot tests with transpose=True. Learn how to run the unit tests (should be in the developer guide) and that will save you a lot of time as you test this.

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.

2. No need to pass transpose to any functions involving gds, eg to_gds. These are used when importing and exporting to GDSII files. https://www.flexcompute.com/tidy3d/examples/notebooks/GDSImport/

Thanks! I'll fix that now.

3. Pretty minor comment but might as well start thinking about the argument naming. I am pretty confident that a bool is the right data type for this option, but the naming I'm not sure about.
   a. in the plotting functions, we could also consider like swap_axes : bool = False?
   b. in the pop_axis and unpop_axis, maybe we consider something similar? or swap_planar_coords?

I will let you decide. But I suggest we use the same name for all functions. (I like both swap_axes and transpose because the names are short compared to swap_panar_coords.)

4. The only thing I'm still not sure about / hesitant about is whether we should be modifying these pop_axis and unpop_axis functions directly. I'm leaning towards thinking it's ok, but another way we could approach this to introduce new functions pop_axis_and_swap(coords, axis, swap_axes: bool) that simply calls pop_axis(coords, axis) and swaps the results if swap_axes is true.). Something to think about, but not sure if it improves anything or just complicates things.

I was wondering about this too. I will look into that.

Thanks for the effort on this! good luck on the testing.

Thank you!

[jewettaijfc]

Currently:

• import tidy3d works without crashing
• Box.plot(), is working (with transpose=New)
• Scene.plot(), is working (with transpose=New), except when the scene contains Polyslabs (See below)

Here are some things fixed in this PR (which weren't working before in PR2537):

• Monitor.plot() is working (with transpose=True).
• Source.plot() is working (with transpose=True).
• plot_arrow() is working (with transpose=True).
• When hlim and vlim are unspecified (=None), the boundaries are correctly swapped when transpose=True. (This is what we want to happen.)
• Simulation.plot_grid() seems to be working (with transpose=True). This needs more testing...

Here are some things that are currently broken in this PR (which were working in PN2537):

• Transposing Polyslabobjects does not work yet. But I know why: I haven't made any changes to thegeometry/polyslab.py` file yet. I'll work on that tomorrow.

[jewettaijfc]

• PolySlabs are working with transpose=True. (I had to add a transpose argument to PolySlab._intersect_normal() and PolySlab._intersect_side().)
• I will test cylinders next.

NOTE: I left some print() statements in the code that I'm still using for (manual) debugging. I'll remove these soon.

[jewettaijfc]

• Cylinders are working with transpose=True

[jewettaijfc]

I'm done with manual testing.

I started writing unit tests.

[jewettaijfc]

Oops. I guess I wasn't done with manual testing.
I fixed a bug and (visually) verified that

• Simulation.plot_eps() is working (with both transpose=True and False).
• Now all of the functions in VisSimulation.ipynb are working (with both transpose=True and False).

[jewettaijfc]

• HeatChargeSimulation.plot_sources() is now working when swap_axes=True and false
• The HeatSolver.ipynb notebook is working now

[jewettaijfc]

• The HeatDissipation.ipynb notebook (from the tidy3d-notebooks examples) also seems to be working ...up until the point where I have to submit simulations. (I can't run the remaining portion of the .ipynb notebook because it looks like my free tidy3d account has expired. I'm working on fixing this.)

[tylerflex]

I think this is causing the error, this line should be put back

[tylerflex]

yup! just verified that it works after adding it back

[jewettaijfc]

Wow. Thank you! This would have taken me hours to figure out.

[tylerflex]

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 axis

[jewettaijfc]

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.

[tylerflex]

good catch, this is probably more robust than what we had before.