-
Notifications
You must be signed in to change notification settings - Fork 47
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
Error if projection monitor touches periodic/bloch boundaries in 3D simulations #1958
base: develop
Are you sure you want to change the base?
Conversation
01eff25
to
f465496
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.
Now that I think about this, I think field projection monitors will always be wrong if there are periodic or Bloch boundaries, regardless of whether they touch the boundaries or not. Either way, to do a correct projection, a periodic Green's function must be used. So I think the error should just be whenever there's any field projection monitor and any periodic/bloch boundaries together - for a 3D simulation, I cannot come up with a case in which this will work well even if e.g. the monitor is along xy and the boundaries are along z, it's again weird. And the error could suggest to the user to look into DiffractionMonitor, which is probably what they want to use in most cases, unless they have periodicity on one axis and not the other - that's something we don't really support now.
@@ -2494,6 +2494,47 @@ def boundaries_for_zero_dims(cls, val, values): | |||
|
|||
return val | |||
|
|||
@pydantic.validator("monitors", always=True) | |||
@skip_if_fields_missing(["size", "boundary_spec"]) |
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 guess also "center"?
Ah, I see. This makes sense. Previously, I thought projection with just one period was physically meaningful, but yeah it's not practically meaningful. |
f465496
to
f0c112a
Compare
@momchil-flex I updated the code to throw an error when a FieldProjectionMonitor is used with periodic/bloch boundaries. But it appears this sim object is used in many test functions and will throw a bunch of errors (some related to DiffractionMonitor) if I change the boundaries from periodic/bloch to PMLs. Could you give me some suggestions for the fix? |
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.
Hm, yeah, I see the issue - not sure, will open it up for discussion.
if isinstance(monitor, AbstractFieldProjectionMonitor) and is_3d_simulation: | ||
raise SetupError( | ||
f"Using 'FieldProjectionMonitor' '{monitor.name}' " | ||
f"with periodic/Bloch boundaries along the {axis} axis would lead to incorrect results. " |
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 no need to print the axis as using the projection monitor with any periodic/Bloch boundaries is wrong.
Using 'FieldProjectionMonitor' '{monitor.name}' with periodic/Bloch boundaries will lead to incorrect results.
This validator raises an error in 3D simulations when a projection monitor touches periodic/Bloch boundaries. For 2D simulations, we allow periodic boundaries, which won't trigger this error. For 2D with Bloch boundaries, we already error it out in another PR where we just don't allow Bloch boundaries along the 0-dimension.