-
Notifications
You must be signed in to change notification settings - Fork 40
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
Improved warnings with Medium2D #1097
Conversation
e444f04
to
4ccf3f9
Compare
4ccf3f9
to
0b91e1d
Compare
@caseyflex I'm a bit confused. This seems to break both the regular and 2D adjoint tests with an error
Mediums shouldn't have sizes so that's strange. Also, the adjoint tests don't have 2D geometries in them (I just verified on the regular test_adjoint_pipeline) It seems this warning was also triggered in the tests as I see the warning message about 2D structure detected with no 2D medium. Something seems strange here, I'm not sure what to make of it. |
@@ -99,14 +101,24 @@ def eps_diagonal(self, frequency: float, coords: Coords) -> Tuple[complex, compl | |||
@pydantic.validator("medium", always=True) | |||
def _check_2d_geometry(cls, val, values): |
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 can't help but wonder if this can just be simplified a lot
geom = values.get("geometry")
if geom is None:
raise SetupError("'Structure.geometry' did not pass validation, can't check 2D medium".)
if isinstance(val, Medium2D) and not geom.is_2D:
raise SetupError("Structure with a `Medium2D` medium needs a `2D` geometry.")
if geom.is_2D and not isinstance(val, Medium2D):
zero_axis = geom.bounding_box.size.index(0)
log.warning("found a geometry with < 3 dimensions not associated with a Medium2D ...")
with a simple property Geometry.is_2D
that is implemented in each of the geometry subclasses?
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.
Alternatives:
- add a
Geometry.is_3D -> bool
method and checkif not geom.is_3D
- add a
Geometry.ndims -> int
and check ifgeom.ndims < 3
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 the complicated logic is a bit necessary, since we are checking different things. On the one hand, we need to check that if the medium is a Medium2D, the geometry must be a supported type and it must be 2D. Here, _normal_2dmaterial gives a detailed error message depending on the geometry and setup. On the other hand, if the bounding box has zero size in a certain direction and the medium is not Medium2D, it is helpful to give a warning. I'll add some comments to clarify this but I'm inclined to leave the logic as-is
0b91e1d
to
e0ea25b
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.
Ok, the comments do help, looks good to me, thanks.
This adds a warning when a 2D geometry doesn't have a 2D medium.
@tylerflex it seems to break the adjoint tests, which has 0-thickness structures in a 2D sim. Is it possible to change these thicknesses to some other value, like td.inf? This may be more robust anyway without changing the intended behavior.
@tomflexcompute , this should also fix the plot_eps warning