-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Phase 1: self-intersecting polyslab #678
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'm not able to follow everything but I think this general approach makes sense. So if a user wants to use a ComplexPolySlab
in their Simulation
, would the standard way be to
cps = ComplexPolySlab(...)
geo = cps.geometry_group
structure = td.Structure(geometry=geo, medium=medium)
? I worry a little that it won't be so clear to the new user how to do this, so maybe there can be some mention of this in the Docstring of ComplexPolySlab
. For example
Class ComplexPolySlab(PolySlab):
""" Interface for dividing a complex polyslab where self-intersecting polygon can occur during extrusion.
To generate a :class:`.GeometryGroup` from a `ComplexPolySlab`` instance for use in :class:`Simulation`,
pass `complex_polyslab.geometry_group` to the `geometry` field of :class:`.Structure`.
"""
Right, and the other approach is
So how about a docstring like this?
|
I usually find it most useful to have a small example straight into the documentation, much like numpy and scipy do. It's easy to understand and quicker to find than going through a complete example notebook (which should, of course, exist as a full reference and tutorial) |
Related: It might be nice to add def to_structure(self, medium: MediumType) -> td.Structure:
"""Construct a structure containing a user-specified medium and a GeometryGroup made of all the PolySlabs from this object.""" just as one other alternative to both of those. |
Good point. It has been added to the doc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, this should work for edge collapsing (neighboring vertices merging), but I'm not sure it will be straightforward to implement the general case. That's because the subdivision algorithm explicitly uses the offset at which the collapsing event happens, which is much harder for edges. Anyway, for now it looks great!
tidy3d/components/geometry.py
Outdated
vertices = self._proper_vertices(self.vertices) | ||
if isclose(self.dilation, 0): | ||
return vertices | ||
return self._shift_vertices(vertices, self.dilation)[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dilation/erosion might itself result in self-intersecting polygons. Should we do some clean-up to remove those? Here the problem is easier than with the extrusion, since it doesn't have to be compared to any other slabs. @weiliangjin2021 I believe you mentioned that shapely had already a function that could be used for this, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about doing this as well, but then quickly realized that if we allow self-intersecting polygon at the reference plane and perform clean-up, there are situations where the healed polygon can split and have islands/holes. So right now, we simply error in this case:
tidy3d/tidy3d/components/geometry.py
Lines 1854 to 1855 in 94b801b
@pydantic.validator("vertices", always=True) | |
def no_self_intersecting_polygon_at_reference_plane(cls, val, values): |
Another possibility is to error only when the healed polygon splits or generates holes/islands, but allow the change of vertices number. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its usually best to make it easier for the user, as long as we don't overstep on what their intentions might be. In this case, we should gracefully handle both cases, but I can see that the current API might make it difficult to return multiple islands. Maybe continue to error in this case, but handle the easier one, where we just clean-up the polygon and remain with a single proper polygon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we error or just warning for polygon splitting/islands? I'm thinking about: 1) error for polygon splitting/islands; 2) warning for the change of the number of vertices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part has been added. Quite significant change in the two validators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, what I have now doesn't really address your comments, since I still offset the vertices first and then heal. I cannot think of any self-intersecting polygon that can really pass the validator here. Maybe let's plan on the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an useful implementation. With the regular PolySlab
, it seems like as long as the user knows where the vertices will meet during the extrusion, they can always manually define multiple polyslabs to achieve the same final geometry (group) as the ComplexPolySlab
, but it could take a lot of trigometry to figure everything out.
My concern is that polyslabs that will result in self-intersection vertices usually have a more complex geometry. After extrusion, it might be difficult to examine if the result is correct only by examining the crossing section using the plot
method. At this point, probably there is no better way than submitting the job to the server and going to web GUI for visualization.
94b801b
to
63b2b95
Compare
Visualization and GDS import
84903bb
to
e6ffbe2
Compare
The support of self-intersecting polyslabs is scheduled to roll out in two phases. In phase 1, we consider the case where the length of some edges can approach 0 (or neighboring vertices become degenerate) at some point during extrusion. In phase 2, more general types of edge events will be treated. The main idea in this PR is to divide a complex polyslab into many simple polyslabs.
ComplexPolySlab
is defined in plugin. It takes the same fields asPolySlab
. We can consider move this class togeometry.py
. The current design is to leave space for phase 2..sub_polyslabs
to obtain a list of simple polyslabs, or.geometry_group
that wraps up the simple polyslabs in a geometry group.PolySlab
. Here is an example.PolySlab
: previously we cache the polygon on the base, and always use base polygon to construct polygons at other extrusion distance. However, now as we construct simple polyslabs from a complex polyslab, the degenerate vertices of the simple polyslab are removed on the top or the base. So the polygon on the base or the top can be different from the polygon in the between. An appropriate way is to use the polygon in the middle to construct polygons at other extrusion distance.