Bringing back symmetries into the mode solver #201

momchil-flex · 2022-02-05T02:09:25Z

Symmetries specified in a ModeSpec now generally work, but there are a lot of possible edge cases...

Also, I have built release-22.1.4 on the server with all the changes from last week, and set it as default here.

tylerflex · 2022-02-08T22:40:56Z

tidy3d/plugins/mode/mode_solver.py

+        # Truncate according to symmetries
+        solver_coords = [coords.copy() for coords in plane_coords]
+        plane_shape = list(eps_wg_xx.shape)
+        for ic, coords in enumerate(solver_coords):


did pylint let you get away with this?

Oh, no, it does complain

tylerflex · 2022-02-08T22:42:29Z

tidy3d/plugins/mode/mode_solver.py

+            if mode_spec.symmetries[0] != 0:
+                sym_val = mode_spec.symmetries[0]
+                # Pad symmetry-tangential E fields
+                e_full[[1, 2], 1:Nx//2, :, :] = sym_val * e_full[[1, 2], :Nx//2:-1, :, :]


I feel like we should think about generalizing these a bit in a symmetry padding function, it's a bit unwieldy

tylerflex · 2022-02-08T22:43:51Z

tidy3d/plugins/mode/mode_solver.py


            # Rotate back to original coordinates
-            (Ex, Ey, Ez), (Hx, Hy, Hz) = rotate_field_coords(E, H)
+            (Ex, Ey, Ez), (Hx, Hy, Hz) = rotate_field_coords(e_full, h_full)


it seems rotate_field_coords does the same thing to e_full and h_gull
Should we make this a function of a single argument and call it twice?

sounds good

tidy3d/web/config.py

tylerflex

Sorry if these are pedantic comments. I feel like this is probably good enough. But, if we have time, we could refactor a little bit more to help ourselves as we build upon this (or replicate similar things in other places (near2far most notably).)

momchil-flex · 2022-02-14T17:37:55Z

I completely agree about the reorganization. I just wanted to get it done before leaving in case it's needed (by a user). I guess it wasn't. I guess since it doesn't seem urgent I can work on the symmetries further. I like your idea of potentially incorporating the unfolding in the FieldData, will be useful for monitors in simulations with symmetries too.

Also symmetries are always really annoying and edge case-y, e.g. like we discussed for example what happens if someone has put the mode plane in a way such that its center doesn't match a simulation cell, or the discretization has an odd number of cells (the symmetry implementation currently assumes even). And these things are even more problematic if they're using non-uniform mesh. Basically we need to ensure that the discretized gird itself has the required symmetry. Last we talked about this I think we decided to just remove the option for symmetries in the ModeSolver unless e.g. the plane center matches the simulation center in that direction, and the simulation is defined with having the same symmetry. This should ensure that the discretized grid is then also symmetric. It feels a bit restrictive, but I think it covers many use cases.

P.S. Actually I think even this doesn't ensure that the discretized grid would be symmetric because of numerical precision issues when the plane size is "exactly" multiple of the grid step. But we could modify the discretize function for simulations to take symmetries into account and always return an even number of cells.

tylerflex · 2022-02-14T17:41:47Z

One other thing to consider is that the symmetries are specified in both the ModeSpec but also in ModeSolver.simulation.symmetry. So either we need to enforce that these match (and the mode plane is centered on the symmetry plane if specified). Or we should remove the symmetries in the mode spec and just have the symmetries come directly from the Simulation, if applicable.

tylerflex · 2022-02-14T17:42:20Z

I think we should be as restrictive as we need to be to ensure it works properly if only for the most simple cases.

momchil-flex · 2022-02-14T18:09:20Z

Yeah good point they can just be removed from the ModeSpec. I think this means substantial changes to this PR before it can be merged, as I'll need to re-enable the simulation symmetry and set up at least some of the infrastructure to handle it. I guess no rush to merge?

tylerflex · 2022-02-14T19:56:24Z

No rush to merge, except that one commit I made to fix the monitor data size computation. Maybe should have done that on another branch. Do you know how to incorporate that in a different PR or just add it to develop?

momchil-flex · 2022-02-14T20:05:01Z

git checkout develop
git cherry-pick a1532b031ed294d3ce9510984bcdc702097ab03f

tylerflex · 2022-02-14T20:09:46Z

Alright, I just did this in #211. Feel free to merge and then remove my commit in this PR.

tylerflex · 2022-02-14T20:10:25Z

I didnt cherry pick, didnt see your message in time, just made a new branch off develop and checked out that file.

momchil-flex · 2022-02-14T20:12:45Z

That also works for a single file.

momchil-flex · 2022-02-22T00:58:11Z

Symmetries in mode solver now taken from simulation symmetries, if the mode plane lies on the simulation center.
Validator that mode objects with symmetries are either entirely in the main quadrant, or lie on the symmetry axis.
Setting up some infrastructure for handling symmetries in general.
AbstractFieldData now has an apply_syms method which uses interpolation and symmetries to create a new field data on a global grid. This may be refactored a little bit more in the future. What I am considering doing is storing the ScalarFieldData on the smallest main quadrant grid that can fully cover the global (symmetry-unwraped) FieldData dict. In FieldData, I could also store the symmetry properties that the fields were computed with, as well as the unwrapped grid. Then, e.g. FieldData.Ex could return the unwrapped field directly. This can have the advantage for example of always storing the minimum needed data to file.

tylerflex · 2022-02-22T23:37:45Z

tidy3d/components/geometry.py

        """
        return Box(center=self.center, size=self.size)

+    def pop_axis(


not sure I like this.. feel like it clutters things up a bit because it's for such a specific application. How many times is the called and does it make sense to just create the normal_index = size.index(0) and feed to regular pop axis?

It's used a few times in the frontend and a few times in the backend. It seems to me that almost every time I need to use this, I have a plane that I'm working with, so it seemed weird I have to specifically supply the normal index every time. I almost don't understand why this would be a geometry method if not for some simplifications it can provide for geometries that have a natural normal axis...

Well, like you say, it's used in situations where there is a natural normal axis, like cylinders and poly slabs. But note these are not toggled via size. It's also used to compute intersections and polygons in various planes (where the plane axis is specified). So my point is that these functions are pretty generically used and I would prefer to not hardcode something for a special case if we dont have to. If it were me I'd just construct and feed it the normal axis. Or, I might try to handle it a bit differently (like define a normal_axis property in each Geometry that returns a value used as a default in pop / unpop axis.

So I can partly understand your resistance to this, but firstly note that the change I made doesn't brake anything as the size selection is only triggered if axis is not supplied. There are two things that feel kinda strange to me: the first is that you have to manually define the normal axis every time even for geometries that have a natural normal axis. The second one is that it's not really clear why the method as such (staticmethod) belongs in the Geometry class (perhaps the best justification is that we don't really have hanging functions in something like a utils module). But I actually like your idea about defining a normal_axis property! This can be set to the smallest size for a Box and the extrusion direction for Cylinders and Polyslabs. Then we only need to change the Geometry pop/unpop methods to allow axis=None, and raise an error if axis == None and self.normal_aixs == None (e.g. for a sphere). What do you think?

To your first point, it's because you dont always supply a normal axis to the pop_axis function, it could be anything, like the plot axis or whatever. Point is that it's generic and doesn't care about the context it's being called in.

It might not belong in the Geometry class, it only does because I feel like it's better tied to some class rather than just hanging out in a utils file, and 2nd is that it's really only used for Geometry objects (things that can be plotted, intersected, etc.). Other than that it's an arbitrary choice.

Normal axis property would work like you describe yea, but I still am a bit opposed to it unless we can define what a normal axis is and why pop_axis should have it as a default? Is it just going to be a property defining "one of these axes is not like the other 2"? for a specific geometry? I'm probably being OCD but just want to think through whether we add this if it's just to avoid a few lines in other places where normal_axis is fed. A nice thing about feeding normal axis is it's more explicit and readable.

Is it just going to be a property defining "one of these axes is not like the other 2"? for a specific geometry?

Pretty much, yeah. It seems you already have something like that actually: Planar, where pop_axis is called several times with self.axis.

I'm probably being OCD but just want to think through whether we add this if it's just to avoid a few lines in other places where normal_axis is fed. A nice thing about feeding normal axis is it's more explicit and readable.

This is not a hill I want to have a long discussion on. I can revert the change. It seems that in my usage it was always tied to a plane so it felt weird to have to supply the normal every time, but I see how it's used in the plotting functions. I mean, I didn't get your point to my first point - what I was saying is that everywhere in the code up to now the axis is specifically supplied, so my change doesn't break anything. But I also see that it's not great the way it is now. So between making a normal_axis property for all geometry and just reverting the change, I'm fine with reverting.

Ok, yea if you dont mind reverting that would make me most happy :). But we can revisit this issue. What makes me feel a bit icky is adding specific behavior (like checking size) to this more generic, core function that is used in a bunch of other ways. It wouldn't break anything, but I also have been trying to avoid this out of principle in the code design (similar idea to how the components should not depend on the plugins).

tylerflex

Just a few comments. Looks overall good

tylerflex · 2022-02-22T23:39:25Z

tidy3d/components/simulation.py

    """ Validating setup """

-    @pydantic.validator("symmetry", always=True)
-    def not_supported_yet(cls, val):


so as of this PR symmetry is only supported for mode computation? Or am I missing something?

Then we should edit the docstring for Simulation.symmetry temporarily?

Well I am still hoping to get the main symmetries done in the next couple days... Sooo no need?

tylerflex · 2022-02-22T23:41:55Z

tidy3d/components/validators.py

+                    "is completely outside of simulation domain."
                )
+
+            if geometric_object.type in ["ModeSource", "ModeMonitor"]:


seems like this should be a separate validator?

I guess it can be yeah

… some bugs

momchil-flex requested a review from tylerflex February 5, 2022 02:16

tylerflex reviewed Feb 8, 2022

View reviewed changes

tidy3d/web/config.py Show resolved Hide resolved

tylerflex requested changes Feb 8, 2022

View reviewed changes

momchil-flex force-pushed the momchil/mode_solver branch 3 times, most recently from f3741ea to 8c7e76e Compare February 22, 2022 00:48

momchil-flex force-pushed the momchil/mode_solver branch from 8c7e76e to ad907b5 Compare February 22, 2022 01:01

tylerflex reviewed Feb 22, 2022

View reviewed changes

momchil-flex added 4 commits February 24, 2022 12:26

Bringing back symmetries into the mode solver

eaab741

Reorganizing grid boundary generation to handle symmetries and to fix…

820c408

… some bugs

Mode solver symmetries reorganized to come from simulation symmetries

5a9c100

Reverting pop axis for box and splitting the mode symmetry validator

1f5ca74

momchil-flex force-pushed the momchil/mode_solver branch from ad907b5 to 1f5ca74 Compare February 24, 2022 20:37

momchil-flex and others added 2 commits February 24, 2022 13:15

Pylinting viz.py and removing chart_studio as a requirement

be22e53

fixed plotly and tested, blacked

079df12

tylerflex merged commit 60bdb9a into develop Feb 24, 2022

Bringing back symmetries into the mode solver #201

Bringing back symmetries into the mode solver #201

Uh oh!

Conversation

momchil-flex commented Feb 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tylerflex left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

momchil-flex commented Feb 14, 2022

Uh oh!

tylerflex commented Feb 14, 2022

Uh oh!

tylerflex commented Feb 14, 2022

Uh oh!

momchil-flex commented Feb 14, 2022

Uh oh!

tylerflex commented Feb 14, 2022

Uh oh!

momchil-flex commented Feb 14, 2022

Uh oh!

tylerflex commented Feb 14, 2022

Uh oh!

tylerflex commented Feb 14, 2022

Uh oh!

momchil-flex commented Feb 14, 2022

Uh oh!

momchil-flex commented Feb 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

momchil-flex Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerflex Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerflex left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tylerflex left a comment •

edited

Loading

momchil-flex Feb 22, 2022 •

edited

Loading

tylerflex Feb 22, 2022 •

edited

Loading