-
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
Updated the medium intersection validator to handle 2D projection monitors without raising errors. #1907
base: develop
Are you sure you want to change the base?
Conversation
4588a6e
to
d2d074b
Compare
c518c6f
to
866657f
Compare
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.
Thanks @QimingFlex, just a few comments. Might want to get @dbochkov-flexcompute to take a quick look just to make sure this code doesn't have any unintended consequences in the Scene
.
tidy3d/components/scene.py
Outdated
@@ -234,6 +234,8 @@ def intersecting_media( | |||
Object for which intersecting media are to be detected. | |||
structures : List[:class:`.AbstractMedium`] | |||
List of structures whose media will be tested. | |||
zero_dim_ind : int |
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.
technically in the docstrings for optional arguments (with defaults) we'd put the default in here like
zero_dim_ind : int = None
tidy3d/components/scene.py
Outdated
exclude_surfaces_2d: List[str] = [] | ||
if zero_dim_ind is not None: | ||
exclude_surfaces_2d = {0: ["x-", "x+"], 1: ["y-", "y+"], 2: ["z-", "z+"]}.get( | ||
zero_dim_ind, [] |
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 suppose this could just be a list?
[["x-", "x+"], ["y-", "y+"], ["z-", "z+"]][zero_dim_ind]
Also if the zero_dim_ind is not found, we probably just want to error here right? instead of returning an empty list?
tidy3d/components/simulation.py
Outdated
@@ -2707,7 +2714,10 @@ def _projection_monitors_homogeneous(cls, val, values): | |||
with log as consolidated_logger: | |||
for monitor_ind, monitor in enumerate(val): | |||
if isinstance(monitor, (AbstractFieldProjectionMonitor, DiffractionMonitor)): | |||
mediums = Scene.intersecting_media(monitor, total_structures) | |||
if isinstance(monitor, AbstractFieldProjectionMonitor): |
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.
do we need this if else
what is different about the diffraction monitor?
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 that this PR may only be fixing a subclass of the edge cases that are in principle possible. For example, it doesn't even have to be a 2D simulation for an issue to arise: assume you have a structure entirely outside of the (3D) simulation domain - I think currently we allow this with a warning. A projection monitor could also intersect that and the error will be raised even though actually it's ok. Also this would be true for DiffractionMonitor, which in this PR is not addressed at all.
I am also not a fan of modifying Scene.intersecting_media
for this edge case (but also if my described edge case above is true, the zero_dim_ind
modification is not general enough anyway). Maybe a better approach is to make these validators post_init
so that we can compute self.simulation_bounds
; make a Box.from_bounds
using these bounds; and update the monitor we are testing to have center and size that are given by the intersection of its nominal center and size and the simulation Box. Then pass that to intersecting_media
.
This probably should also be done for other homogeneous media validators like for PlaneWave, GaussianBeam, and maybe tfsf? Could you take a look @QimingFlex ?
CHANGELOG.md
Outdated
@@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- Error when plotting mode plane PML and the simulation has symmetry. | |||
- Validators using `TriangleMesh.intersections_plane` will fall back on bounding box in case the method fails for a non-watertight mesh. | |||
- Bug when running the same `ModeSolver` first locally then remotely, or vice versa, in which case the cached data from the first run is always returned. | |||
- Error when field projection monitor intersecting structures outside of the simulation domain in 2D simulations. |
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 should be in Unreleased, not in 2.7.2
Thanks @tylerflex for the comments. Yeah, modifying the |
Thanks @momchil-flex . Yeah, modifying the scene is not a good idea for this corner case. Initially, I took the approach of updating the monitor to pass this test, but it failed in some of the diffraction tests. I'll need some time to update this to be more general, including the 3D case and the Diffraction Monitor you mentioned. |
2095605
to
3e2f044
Compare
Hi @momchil-flex , My thoughts on the thorough fix. There are three key elements in this fix: structures, simulation domain, and projection monitors. Regarding the first two elements (structures and simulation domain), for both 2D and 3D simulations, we must allow structures to extend outside the simulation domain in some cases:
|
@QimingFlex the approach that I propose was in my previous comment
Do you see any issue with this proposed method? Maybe this explanation is not very clear, we can discuss further. |
I believe one problematic case could arise in 2D simulations. When calculating the intersection, the updated monitor's size along the zero-dimension would become 0. This would transform the monitor from its intended four-surfaces structure into a single surface out of simulation domain, which is incorrect. And in general 3D simulations it might still throw errors on surfaces at the simulation edge. |
3e2f044
to
7fda2a7
Compare
@momchil-flex This fix considers the case where a box monitor is given. It checks its bounds against the simulation bounds. If any dimension of the monitor exceeds the simulation domain, we do the following:
This would generally lead to the problem I mentioned above, where a 2D simulation could reduce a box monitor to a surface monitor. To avoid this, I introduced a very small value along the 0-dimension to keep the monitor as a box. Finally this fix is able to prevent errors from being thrown in both 2D and 3D simulations. However, the backend needs to be fixed as well. Do the backend perform some medium checks in C++? Currently, I can't get correct solutions which I believe is related to the wrong medium assigned to the monitor. |
7fda2a7
to
7bfd25e
Compare
I updated the
_projection_monitors_homogeneous
validator to handle the error of medium intersections outside the computational domain in 2D simulations. The original validator checks if each face of a projection monitor intersects more than one medium. In 2D simulations, this was generating errors when faces normal to the 0-dim direction were included in the validation, which is not necessary. I removed those unnecessary faces accordingly for 2d simulations to avoid such errors.