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

Error if projection monitor touches periodic/bloch boundaries in 3D simulations #1958

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

QimingFlex
Copy link
Contributor

@QimingFlex QimingFlex commented Sep 13, 2024

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.

@QimingFlex QimingFlex force-pushed the qiming/periodic_projection branch 4 times, most recently from 01eff25 to f465496 Compare September 15, 2024 03:46
@QimingFlex QimingFlex marked this pull request as ready for review September 15, 2024 06:37
Copy link
Collaborator

@momchil-flex momchil-flex left a 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"])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess also "center"?

@QimingFlex
Copy link
Contributor Author

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.

Ah, I see. This makes sense. Previously, I thought projection with just one period was physically meaningful, but yeah it's not practically meaningful.

@QimingFlex QimingFlex force-pushed the qiming/periodic_projection branch from f465496 to f0c112a Compare October 4, 2024 21:55
@QimingFlex
Copy link
Contributor Author

@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?

Copy link
Collaborator

@momchil-flex momchil-flex left a 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. "
Copy link
Collaborator

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.

@momchil-flex momchil-flex added the 2.8 will go into version 2.8.* label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 will go into version 2.8.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants