correcting plot functions for ModeSolver

[FilipeFcp]

There was a little bug in the _center_and_lims method, so simulations with the center not in origin would plot an incorrect ModeSolver plot.

Also, I made a little adaptation to ensure that the plot functions would plot the entire plane object of the ModeSolver. I believe that the computation will be carried out in the plane right, regardless of whether it is larger than the Simulation object size, is that correct?

I did some tests in this notebook

There seems to be still a little bug with the plot_pml function.

[e-g-melo]

Hi @FilipeFcp! Regarding the size of the mode plane, I understand that we should show the calculation plane for users. However, the fact that it differs from the values they used to define the mode plane can create confusion. If the user sets a 3 x 2 microns plane and visualizes a 3.2 x 2.1 (or other value), that can sound like an error (maybe more critical for GUI users).

@momchil-flex, if we include grid snapping_points on the mode plane lower left and upper right corners, could we make the calculation plane dimension the same as in the component definition?
Side Note: a user complained about the PML lying within the mode plane rather than outside of it. Would it be very hard to change this?

[FilipeFcp]

Hi @e-g-melo, I just spoke with Tom and realized I misunderstood the ModeSolver behavior. I initially thought that if the ModeSolver plane was larger than the simulation size, the calculation would take place on the larger plane.

I've reverted to the current version and just corrected the plane positioning.

[e-g-melo]

Thanks @FilipeFcp! Those questions still remain, though. The simulation plane is usually slightly larger than the mode plane definition by some nanometers (~100-300 nm). If we can make it the same size, that would be interesting.

[momchil-flex]

@momchil-flex, if we include grid snapping_points on the mode plane lower left and upper right corners, could we make the calculation plane dimension the same as in the component definition?

Maybe, I'm not 100% sure if we don't do like an extra pixel in some cases or something like that.

Side Note: a user complained about the PML lying within the mode plane rather than outside of it. Would it be very hard to change this?

Yeah I've thought about this a few times but my conclusion always is that it's unfortunately not that straightforward. We should do it some day.. but since it won't be quick, I'm not sure when.

[FilipeFcp]

Hi @e-g-melo, I believe it should be relatively easy to add this extra space to the plot.

Regarding the PML, would it be possible to include the PML in the ModeSolver.plot, as it is in the Simulation.plot? This way, it might be easier for the user to notice if the PML isn't positioned properly and adjust the simulation using Simulation.updated_copy to accommodate the PML size.

[e-g-melo]

Regarding the PML, would it be possible to include the PML in the ModeSolver.plot, as it is in the Simulation.plot? This way, it might be easier for the user to notice if the PML isn't positioned properly and adjust the simulation using Simulation.updated_copy to accommodate the PML size.

Yes.. that is good!

[FilipeFcp]

@e-g-melo I made a minor adjustment so that the plot method will call the plot_pml. I believe it could be good since the PML positioning is a bit not intuitive. But I can revert to the previous version and just correct for the positioning if you think is better.

[e-g-melo]

Hi @FilipeFcp! It sounds good and will help our users. Thanks

[caseyflex]

I noticed you had removed the absolute value here, but now it is restored. Seems like it shouldn't be here?

[FilipeFcp]

Yes. Initially, I thought the definition of h_lim was causing issues in some situations, but I realized that wasn't the case. Since this definition is equivalent to mine without the absolute value, I decided to keep the original. Do you think I should revise it?

[caseyflex]

If they are equivalent then this is fine. Thanks

…

On Thu, Sep 26, 2024 at 3:20 PM FilipeFcp ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In tidy3d/plugins/mode/mode_solver.py <#1966 (comment)>: > @@ -1472,10 +1474,10 @@ def _center_and_lims(self) -> Tuple[List, List, List, List]: _, (h_min_s, v_min_s) = Box.pop_axis(self.simulation.bounds[0], axis=n_axis) _, (h_max_s, v_max_s) = Box.pop_axis(self.simulation.bounds[1], axis=n_axis) - h_min = a_center[n_axis] - self.plane.size[t_axes[0]] / 2 - h_max = a_center[n_axis] + self.plane.size[t_axes[0]] / 2 - v_min = a_center[n_axis] - self.plane.size[t_axes[1]] / 2 - v_max = a_center[n_axis] + self.plane.size[t_axes[1]] / 2 + h_min = self.plane.center[t_axes[0]] - self.plane.size[t_axes[0]] / 2 + h_max = self.plane.center[t_axes[0]] + self.plane.size[t_axes[0]] / 2 + v_min = self.plane.center[t_axes[1]] - self.plane.size[t_axes[1]] / 2 + v_max = self.plane.center[t_axes[1]] + self.plane.size[t_axes[1]] / 2 h_lim = [ h_min if abs(h_min) < abs(h_min_s) else h_min_s, Yes. Initially, I thought the definition of h_lim was causing issues in some situations, but I realized that wasn't the case. Since this definition is equivalent to mine without the absolute value, I decided to keep the original. Do you think I should revise it? — Reply to this email directly, view it on GitHub <#1966 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3KLECNIJR2CLOAY36UIMODZYP3YNAVCNFSM6AAAAABOQPNT5KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMZRGA3DQNJZGY> . You are receiving this because your review was requested.Message ID: ***@***.***>

[momchil-flex]

I haven't really followed this but @e-g-melo @FilipeFcp if you think this is now done we could merge.

[FilipeFcp]

Hi @momchil-flex, I believe it is. The only modification is that the .plot method for ModeSolver now includes plotting the PML (so does that same as the .plot_pml method). Apart from that, it's just a minor correction.

[e-g-melo]

Hi @FilipeFcp! Since you have included the PML in ModeSolver.plot(), we should remove the structures from ModeSolver.plot_pml() to show only the PML as in Simulation.plot_pml().

[FilipeFcp]

Hi @e-g-melo . I removed the structures for the plot_pml. Now instead of creating the structures, it will just creat the ax object and set the correct limits. Let me know if it needs any modification.

[e-g-melo]

Thanks @FilipeFcp! It seems good!

[momchil-flex]

@FilipeFcp could you just add a changelog item for this please? Or message it to me I need to update the changelog a bit before release anyway.

[FilipeFcp]

Hi @momchil-flex. I would add this under ###fixed: - ModeSolver.plot method when the simulation is not at the origin.