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

Added validation for symmetry along zero size dimension #1344

Merged
merged 1 commit into from
Jan 2, 2024

Conversation

tomflexcompute
Copy link
Contributor

Addressing issue #1343 .

@tomflexcompute
Copy link
Contributor Author

This test_adjoint_pipeline_2d failed. It takes a 3D jax simulation with symmetry (1,-1,0) and updates the size to 0 in y. Isn't it problematic?

@tylerflex
Copy link
Collaborator

Yea actually this might be problematic. This test is just to make sure adjoint runs through with all of the proper settings in the simulation. I just wanted to ensure there were some symmetries present. To fix, could you try setting the symmetry to (1,0,-1) in the simulation instead? We can include that change in this PR. Thanks

@tomflexcompute
Copy link
Contributor Author

Thanks @tylerflex for sharing the information. After a second thought, it might actually be fine to have symmetry in zero size dimension if the boundary is PEC or PMC? In the current validator it only validates against absorbing boundaries so in principle it's fine to have a zero size and PEC or PMC boundaries while having symmetry, although symmetry really does nothing physically. However, we don't validate, for example, if an user uses PEC for z_plus and PMC for z_minus and have size_z=0. Does it make sense to allow it?

Nontheless it seems generally safe to disallow symmetry in zero size dimension as it doesn't really add any value. Maybe @momchil-flex have some additional insights.

@tylerflex
Copy link
Collaborator

We could also consider simply warning if there's symmetry along a size 0 dimension

@tylerflex
Copy link
Collaborator

It might be good to change the adjoint test symmetry either way though as it's ambiguous currently.

@tomflexcompute
Copy link
Contributor Author

Yeah warning might be fine as well. My slight concern is many users ignore warnings by default and proceed to submit the simulation anyway. In other cases such as this errored task yesterday, the user originally was quite convinced that using symmetry was correct. If he saw the warning he probably would've ignored it too. Since symmetry doesn't really provide much value in zero size dimension, not allowing it might be better. However it should be a pretty rare case for a user to set up zero size and symmetry at the same time anyway, so either way would be fine in my opinion.

@tylerflex
Copy link
Collaborator

I think I'm fine either way (warning or error). Just noting for @dbochkov-flexcompute that if we start disallowing (or warning) for symmetry in dimensions where the size==0, we might need to take this into account in the .subsection conversion (if not already)

@tylerflex
Copy link
Collaborator

@tomflexcompute if having the symmetry doesn't affect anything practically in the results, it might be too restrictive to error. If it's just an inefficiency that could indicate some issue with the user's script, warning is probably better. If there are any warnings in running the script, I'd say it should still run, but we highly recommend the user consider them. (eg. a feature is being unsupported in next version, or some combination of specs may lead to unexpected results).

@tomflexcompute
Copy link
Contributor Author

Tested it a bit on the DBR example. Along the zero size dimension (x), PEC boundaries with -1 symmetry does lead to the same result as periodic boundaries with 0 symmetry. This is expected since the source is a plane wave polarized in x. PEC is physically equivalent to periodic.

Periodic boundaries with -1 symmetry actually work too without error. Periodic boundaries with +1 symmetry led to incorrect results but didn't error either. Not sure why in the user's case it errored. Maybe it has to do with the specific source?

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.

looks good, let's wait for more discussion to decide exactly how to handle this. in the meantime, could we also add

  1. a line to "Added" in changelog. About more robust validation for symmetry when simulations are < 3d or something similar.
  2. change the symmetry in the make_sim() function of the adjoint plugin tests so that the symmetries are only applied in the size > 0 dimensions.

Thanks @tomflexcompute !

f"The simulation has zero size along the {'xyz'[dim]} axis, so "
"using symmetry along that axis is likely not necessary and "
"could potentially lead to incorrect results. "
f"Consider setting symmetry to `0` along {'xyz'[dim]}."
Copy link
Collaborator

Choose a reason for hiding this comment

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

symmetry -> 'Simulation.symmetry' might make it more clear which field specifically to set.

num_absorbing_bdries = sum(isinstance(bnd, AbsorberSpec) for bnd in boundary)
if num_absorbing_bdries > 0 and size_dim == 0:
raise SetupError(
f"The simulation has zero size along the {'xyz'[dim]} axis, so "
"using a PML or absorbing boundary along that axis is incorrect. "
f"Use either 'Periodic' or 'BlochBoundary' along {'xyz'[dim]}."
)

if symmetry_dim != 0 and size_dim == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to add a condition to actually error if Periodic boundaries with +1 symmetry (as discussed?) We can wait for others to chime in.

@tomflexcompute
Copy link
Contributor Author

looks good, let's wait for more discussion to decide exactly how to handle this. in the meantime, could we also add

  1. a line to "Added" in changelog. About more robust validation for symmetry when simulations are < 3d or something similar.
  2. change the symmetry in the make_sim() function of the adjoint plugin tests so that the symmetries are only applied in the size > 0 dimensions.

Thanks @tomflexcompute !

Thanks @tylerflex . The symmetry in the adjoint test has been changed from the y direction to the x direction.

@momchil-flex
Copy link
Collaborator

Catching up on this now. I think I'm rather in favor of erroring for the case of symmetry + 0D size. We know that at least one job errored on the server because of this, and we don't know why it happened, so this would prevent that from happening again.

Note that a symmetry plane is exactly equivalent to a PEC boundary (-1 symmetry) or a PMC boundary (+1 symmetry) at the center of the simulation. For a 0D size, this basically means that PEC boundary and -1 symmetry should be equivalent. In practice, there's probably some extra symmetry handling (since usually we need to e.g. take only half of the grid steps, etc) that could cause an error. So I think PEC/PMC boundaries for 2D sims make sense as a way to define TM/TE polarized simulations (although as @tomflexcompute points out, we should then also validate that they are the same on both sides; in general a 0D sim should have the same BCs on both sides, i.e. periodic + PEC should probably also be not allowed). The symmetry on the other hand doesn't add any functionality, and may instead not work as expected in the solver, which is why I'm in favor of erroring. We could even say in the error message to use PEC/PMC boundaries instead of symmetry.

@tylerflex
Copy link
Collaborator

erroring is fine by me

@tomflexcompute
Copy link
Contributor Author

So in this version, I changed back to error if symmetry is not 0 alone the 0D axis. Also added a check for whether the plus and minus boundaries match to prevent PEC on one side and PMC on the other side.

f"The simulation has zero size along the {axis} axis, so "
"using symmetry along that axis is incorrect. Use 'PECBoundary' "
"or 'PMCBoundary' to select source polarization if needed and set "
f"Simulation.symmetry to `0` along {axis}."
Copy link
Collaborator

Choose a reason for hiding this comment

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

'Simulation.symmetry' to '0' along

Or just to 0 along, I am not sure if we have specific guidelines for numericals. However we do not use markdown notation like

`0`

in error messages.

@momchil-flex momchil-flex changed the base branch from pre/2.6 to develop January 2, 2024 21:11
@momchil-flex momchil-flex changed the base branch from develop to pre/2.6 January 2, 2024 21:11
@momchil-flex
Copy link
Collaborator

Looks good to me now, squash and merge after one small change. However I think this should go in develop, no need to postpone for 2.6. One way to fix this could be: squash into a single commit; create a backup branch; force reset this branch to develop; and cherry-pick the single commit from the backup branch, then update this PR. Let me know if you want to give it a try, or let me handle it instead.

@tomflexcompute tomflexcompute force-pushed the tom/validate_symmetry_for_zero_dim branch from 8384028 to 7070d5f Compare January 2, 2024 21:23
@tomflexcompute
Copy link
Contributor Author

Looks good to me now, squash and merge after one small change. However I think this should go in develop, no need to postpone for 2.6. One way to fix this could be: squash into a single commit; create a backup branch; force reset this branch to develop; and cherry-pick the single commit from the backup branch, then update this PR. Let me know if you want to give it a try, or let me handle it instead.

Thanks. Sounds quite complex so I'll just have you do it in case I make any mistakes 😏

- Added validation for symmetry along zero size dimension.

- Added validation for unmatching boundary conditions along zero size dimension.
@momchil-flex momchil-flex force-pushed the tom/validate_symmetry_for_zero_dim branch from 7070d5f to 10d1994 Compare January 2, 2024 23:08
@momchil-flex momchil-flex changed the base branch from pre/2.6 to develop January 2, 2024 23:08
@momchil-flex momchil-flex merged commit c21dde3 into develop Jan 2, 2024
2 checks passed
@momchil-flex momchil-flex deleted the tom/validate_symmetry_for_zero_dim branch January 2, 2024 23:18
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

3 participants