Skip to content
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

Reduced sim mode solver #1316

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Conversation

dbochkov-flexcompute
Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute commented Dec 15, 2023

Implements #1191. That is, there is a new simulation method .subsection(region: Box, ...) that can be used to create a smaller simulation based on original one.

  • Most of simulation fields are inherited, but could be optionally overridden
  • Grid can be requested to be the same as in the parent simulation (grid_spec="identical"). In this case it's converted into a custom grid and the requested region is expanded to match parent grid lines. One small issue here is that if the parent grid already matches the requested region, than due to numerical precision one additional cell might be included. But that doesn't seem cause any real issues.
  • Similarly, due to numerical precision issues, even when child grid is essentially the same as the parent grid, some of structures might become slightly misaligned with grid lines. Most prominently it shows when calling ModeSolver._solver_eps() of local mode solver. That is, effective boundaries of structures might differ by one pixel between parent and child simulations.
  • Regarding inheriting symmetry: we discussed in Reduce simulation file size for mode solver #1191 that probably we can just not allow selecting a not compatible region which is not centered around symmetry plane. But I realized this could happen in the background of a remote mode solver call and that would cause an error. So, I implemented it in a way so that an incompatible region is automatically expanded to be centered around a symmetry plane.
  • .subsection(region: Box, ...) is used internally in mode solver web api call and whether it is used or not is controlled by flag reduce_simulation. I set it by default to True, that is, all mode solver calls will use a reduced simulation. We can change it either to False, or make it so that only simulation containing custom mediums are reduced and also display a warning/info notifying that this is done. What do you think?
  • This does significantly help to reduce data transfer for remote mode solver. For example, in this notebook https://github.com/flexcompute-readthedocs/tidy3d-docs/blob/096b84d99d301de4e3690fb6b4aa373064a2bb91/docs/source/notebooks/ThermallyTunedRingResonator.ipynb I couldn't really use remote mode solver because each solve would require around 500mb of upload, now it's less than 1mb. I also expect it to be useful for regular optic simulations in the context of heat solver. I do use it in the same notebook to remove irrelevant structures from optic simulation, but I anticipate more impactful usage eventually. (in this notebook, we are already restricted to a single component. Imagine performing heat simulation on a large scale circuit and then use .subsection() to cut out individual components for optic simulations)
  • On basic level, this feature extends method .sel_inside() from SpatialDataArray to other spatial data arrays (ScalarFieldDataArray, ScalarFieldTimeDataArray), custom medium classes and space time modulation classes. I realize that the name I used .sel_inside(bounds: Bound) might be a bit misleading, because we extract data not just inside but that covers bounds. An alternative could be .reduce_to_cover(bounds: Bound), .sel_to_cover(bounds: Bound). Any other suggestions?

I put three of you as reviewers, but I don't think all of you need to review the entire thing. Maybe @weiliangjin2021 can looks through changes to custom medium classes and modulation classes, while @tylerflex and/or @momchil-flex can comment on overall implementation?

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Thanks @dbochkov-flexcompute I think the implementation makes a lot of sense. I just had a few minor comments about some of the conversions. This will be a nice feature to have in ways we haven't anticipated yet, I'm sure!

tidy3d/plugins/mode/mode_solver.py Outdated Show resolved Hide resolved
@@ -64,6 +65,9 @@ def run(
Optional callback function called when uploading file with ``bytes_in_chunk`` as argument.
progress_callback_download : Callable[[float], None] = None
Optional callback function called when downloading file with ``bytes_in_chunk`` as argument.
reduce_simulation : bool = True
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably a good idea to do it this way.

tidy3d/components/simulation.py Outdated Show resolved Hide resolved
tests/test_plugins/test_mode_solver.py Outdated Show resolved Hide resolved
@tylerflex tylerflex removed the request for review from daquinteroflex December 20, 2023 16:46
tidy3d/components/medium.py Outdated Show resolved Hide resolved
for pole, residue in self.poles:

if not pole.does_cover(bounds=bounds):
log.warning("Pole spatial data array does not fully cover the requested region.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it is necessary to warn at all. It is expected that some structure will be outside the requested region. It might raise tons of warning from a dispersive custom medium.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, here bounds would represent intersection of region from .subsection(region=region, ...) and the bounding box of underlying structure or simulation itself (if background medium). So, if the original structure was put together correctly (that is, custom medium either covers it entirely or only one point is provided along a given dimension for constant extrapolation), then it won't produce any warnings

@@ -3843,6 +4167,9 @@ def eps_dataarray_freq(
for ind, mat_component in enumerate(self.components.values())
)

def _sel_custom_data_inside(self, bounds: Bound):
return self

Copy link
Collaborator

Choose a reason for hiding this comment

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

The reduction doesn't seem to apply to anistoropic custom medium?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does apply. It's just a proper sel_inside() is already inherited from AnisotropicMedium and we just need to neutralize abstract _sel_custom_data_inside inherited from AbstractCustomMedium

@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/reduced-sim-modesolver branch 2 times, most recently from d271522 to 2d6bd78 Compare January 2, 2024 23:42
@dbochkov-flexcompute
Copy link
Contributor Author

addressed all comments, plus:

@momchil-flex
Copy link
Collaborator

This is a good question. I think there are two issues, both of which probably lie outside the scope of this PR.

  • eps_model works for a list of frequencies in some cases, and not in others. I encountered this before as well, in fact I think I fixed it for anisotropic media, but it's still an issue for custom media. The method docstring actually suggests that only a float should be passed, but I believe there are many instances where we pass an array instead. Ideally we should make this consistent across all media, or it will keep popping up.
  • We have a validator that is meant to validate that a diffraction monitor is in a homogeneous medium. However, what it actually does is compute the list of mediums that the plane intersects, and make sure that there's only one element in that list. This does not work for custom medium, which is a single medium which may nevertheless still be inhomogeneous physically. This same issue applies to the validator for plane wave/gaussian beam sources, as well as, in some sense, to the [TFSF validator](def _validate_tfsf_structure_intersections) about structure intersections. I am not sure what's a good way to fix this: it seems like we should either prohibit custom medium, or, if custom medium found, actually sample the permittivity and check if it is homogeneous?

@tylerflex tylerflex self-requested a review January 4, 2024 01:22
@dbochkov-flexcompute dbochkov-flexcompute force-pushed the daniil/reduced-sim-modesolver branch 2 times, most recently from 5496208 to 76559d3 Compare January 4, 2024 23:19
@momchil-flex momchil-flex merged commit 7d2c9a2 into pre/2.6 Jan 5, 2024
12 checks passed
@momchil-flex momchil-flex deleted the daniil/reduced-sim-modesolver branch January 5, 2024 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants