Momchil/nonuniform #314
Momchil/nonuniform #314
Conversation
There was a problem hiding this comment.
I really like the API, really no comments there.
There are a bunch of minor things related to the docstrings / type annotations etc. that can be cleaned up.
I think the auto meshing stuff needs to be refactored a bit to make it more readable and we should avoid free floating functions in favor of static methods. The idea is to make these functions more accessible ie AutoGrid.parse_structure instead of remembering where to import them from in the future.
Overall looks great, really excited for this feature!
| return Grid(boundaries=boundaries) | ||
|
|
||
| # Add a simulation Box as the first structure | ||
| structures = [Structure(geometry=self.geometry, medium=self.medium)] |
There was a problem hiding this comment.
there's a @Property self.background_structure that does this now
There was a problem hiding this comment.
Actually, this doesn't work, because the box in that property has infinite size. Here we actually make a structure with the Simulation.geometry, which is used in make_grid. I think there's a need for both definitions of the backgroudn.
There was a problem hiding this comment.
k, yea let's make it a property then?
There was a problem hiding this comment.
maybe self.structure (singular?)
There was a problem hiding this comment.
I'm a bit worried about future confusion with background_structure.
There was a problem hiding this comment.
ok feel free to rename these or leave one of them out.
tidy3d/components/grid/auto_grid.py
Outdated
| @@ -0,0 +1,770 @@ | |||
| """ Collection of functions for automatically generating a nonuniform grid. """ | |||
There was a problem hiding this comment.
this file is pretty intense.. A couple general comments, I can't follow everything:
- We typically dont like to have free-floating functions when we can help it. I think these should be
@staticmethods of theAutoGrid. - The logic should be broken up into more bite sized chunks. Is there a way to refactor things a bit so the functions are not so enormous? it's definitely annoying and more work, but I think this would probably make it a lot easier to digest.
momchil-flex
force-pushed
the
momchil/nonuniform
branch
from
b6ea730
to
1b8c8b4
Compare
tidy3d/components/simulation.py
Outdated
| ax.add_collection(line_segments_x) | ||
| ax.add_collection(line_segments_y) | ||
|
|
||
| # Plto bounding boxes of override structures |
tidy3d/components/simulation.py
Outdated
| ax.add_collection(line_segments_x) | ||
| ax.add_collection(line_segments_y) | ||
|
|
||
| # Plto bounding boxes of override structures | ||
| patch_kwargs = { |
There was a problem hiding this comment.
the way the plotting is set up now it might be good to define these here
https://github.com/flexcompute/tidy3d/blob/develop/tidy3d/components/viz.py#L108
import them in this file, and then call
plot_params_override_structures.to_kwargs()
and pass zorder in Rectangle
There was a problem hiding this comment.
viz.py
# add `fill : bool = True` to `PlotParams`
plot_params_override_structures = PlotParams(linewidth=.4, edgecolor='black', fill=False)simulation.py
from .viz import plot_params_override_structures
# ...
plot_params = plot_params_override_structure.copy(deep=True)
plot_params.include_kwargs(lw=2 * kwargs["linewidth"], ec=kwargs["colors"])
patch_kwargs = plot_params.to_kwargs()
e35fcc4
to
502c7a4
Compare
… seems sufficiently fast
…array dl to not necessarily have a boundary at the simulation center
…part from IO work
…s out of the class
…lambda0 definition to just use sources' freq0 in validators
momchil-flex
force-pushed
the
momchil/nonuniform
branch
2 times, most recently
from
89b9f88
to
f545b67
Compare
momchil-flex
force-pushed
the
momchil/nonuniform
branch
from
f545b67
to
ad52efd
Compare
No description provided.