Skip to content

Gap automeshing #2390

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 4 commits into
base: daniil/min_size_autodetect
Choose a base branch
from

Conversation

dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Apr 17, 2025

Integrated gap meshing in an iterative way. Now LayerRefinementSpec has two additional parameters:

  • gap_meshing_iters: NonNegativeInt = 1 to set the number of iteration to perform for attempting resolving all gaps
  • dl_min_from_gap_width: bool = True whether or not to reduce dl_min reduce to the minimal detected gap width.

A few of points to discuss:

  • Doing just one iterations seems to be performing quite well, so that could be a good default. But maybe we should switch to a higher number and just let it iterate until no multiple crossings are detected in a single cell?
  • Currently it does not distinguish between thin gaps and thin strips. Do we want to control mesher behavior for the two types
  • while we use snapping planes for achieving the desired result, we currently do not store them, and, as a consequence, we cannot plot them. Should we worry about that?

examples of meshing thin strips and gaps:
Screenshot_20250417_231504
Screenshot_20250417_231652

@dbochkov-flexcompute
Copy link
Contributor Author

one more point:

  • currently we also take into account all structures. Should we worry about making it work for a selected set of structures in this PR?

@weiliangjin2021
Copy link
Collaborator

Thanks for this great feature!

Doing just one iterations seems to be performing quite well, so that could be a good default. But maybe we should switch to a higher number and just let it iterate until no multiple crossings are detected in a single cell?

Agree that we can set 1 as default if ti works quite well already. Then we can add an option like inf or a string to indicate it will iterate until no crossings are detected?

Currently it does not distinguish between thin gaps and thin strips. Do we want to control mesher behavior for the two types

Right now, we don't have good conformal scheme for thin strips, so probably no need right now.

while we use snapping planes for achieving the desired result, we currently do not store them, and, as a consequence, we cannot plot them. Should we worry about that?

Not too concerned, but I wonder that it's not hard to store them?

currently we also take into account all structures. Should we worry about making it work for a selected set of structures in this PR?

My understanding is that metallic structures are more of a concern. So we probably only need to distinguish between metallic (PEC, lossyMetal) and dielectric.

Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Half way through the code. Looks quite nice, although taking quite a bit efforts in understanding the logic. Some high-level description in each function will be very helpful. Some minor comments so far:

"The underlying algorithm detects gaps contained in a single cell and places a snapping plane at the gaps's centers.",
)

dl_min_from_gap_width: bool = pd.Field(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have dl_min from quite a few places: gap refinement, corner refinement, thickness refinement, and minimal feature, shall we allow controling them individually and combine them into a new class?

# reuse the same command but flip dimensions
h_cells_ij, h_cells_dx = self._find_vertical_intersections(y, x, vertices[:, [1, 0]])
if len(h_cells_ij) > 0:
h_cells_ij = np.roll(h_cells_ij, axis=1, shift=1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more explanation here?

@dbochkov-flexcompute
Copy link
Contributor Author

sprinkled around some more comments in the code, had to recall myself what I was doing 😅

Copy link
Contributor

@dmarek-flex dmarek-flex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another great feature! I found a couple of small things in the associated comments. The only real sticking point for me is that it would be nice to get a symmetric grid, when the structure itself is symmetric.

I think it came up in the meeting and maybe there is no easy solution. But for the structure below it seems to always give an asymmetric grid. I thought maybe part of the reason could be due to how intersections are found with grid boundaries, which does not make use of a tolerance. The grid boundary and the polygon vertex might be very close.

image

And here is the original grid without gap refinement. (setting gap_meshing_iters=0)

Screenshot from 2025-04-23 15-28-32

If `None`, recomputes internal override structures.
internal_snapping_points : List[CoordinateOptional]
If `None`, recomputes internal snapping points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing some of the parameters in the docstring

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like missing parameters in the original function's docstring as well

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

periodic and min_dl_from_gaps


# loop over all shapes
for mat, shapes in merged_geos:
if not mat.is_pec:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should support LossyMetalMedium as well, I need to make the same check in my next PR. Maybe we can add is_metal_conductor to AbstractMedium

@@ -1497,6 +1534,253 @@ def _override_structures_along_axis(
override_structures += refinement_structures
return override_structures

def _find_vertical_intersections(self, x, y, vertices):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add some type hints for the return value of these methods


# generate horizontal snapping lines
snapping_lines_y = []
if len(v_cells_ij) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems possible to avoid duplicating the code for the horizontal and vertical lines (as you did for _find_vertical_intersections

cells_dy = []

# for each polygon vertex find the index of the first grid line on the right
grid_lines_on_right = np.argmax(x[:, None] > vertices[None, :, 0], axis=0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to use a small tolerance, so that we can distinguish between almost exactly coincident with the cell boundary and certainly greater than.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might minimize the number of false positives, could also result in some of the asymmetry present in the resulting grids.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants