Skip to content

Giving opposite boundaries different names no longer causes a symmetry validator failure #2682

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

Merged
merged 1 commit into from
Jul 21, 2025

Conversation

jewettaijfc
Copy link
Contributor

@jewettaijfc jewettaijfc commented Jul 18, 2025

This small PR solves issue #2635 .

  • The validator ignores the name attribute of the plus and minus boundary conditions, when deciding whether plus and minus are identical.
  • Previously the validator would fail if the boundaries were given different names.
  • Fixes the original unit test.

Test instructions

Automatic Tests

pytest -s -n0 tests/test_components/test_simulation.py::test_validate_symmetry_boundaries

Manual tests

To test manually, run the following code snippet and confirm that no errors are generated:

import tidy3d as td

_ = td.Simulation(
    size=(1, 1, 1),
    symmetry=(1, 1, 1),
    grid_spec=td.GridSpec.uniform(dl=0.1),
    run_time=1e-12,
    boundary_spec=td.BoundarySpec(
        x=td.Boundary.pml(),
        y=td.Boundary.pml(),
        z=td.Boundary(
            # Give the plus and minus boundaries different names to confirm it does not matter.
            plus=td.PML(name="b1"),
            minus=td.PML(name="b2"),
        ),
    ),
)

(Previously, the _validate_boundary_spec_symmetry() validator would fail.)

Greptile Summary

This PR fixes a bug in the boundary condition symmetry validation logic. Previously, when users set different names for physically identical boundary conditions on opposite sides (plus/minus), the validator would incorrectly fail. The fix modifies the validation to ignore the name attribute when comparing boundaries, focusing only on their physical properties.

The changes involve:

  1. Updating the equivalent() function to make copies of boundaries with empty names before comparison
  2. Using Pydantic's immutable copy(update=...) method to properly handle the comparison
  3. Adding appropriate changelog entry under the 'Fixed' section

This change improves usability by allowing users to give meaningful names to their boundary conditions without triggering false validation errors, while still maintaining proper symmetry validation for the physical properties that matter.

Confidence score: 5/5

  1. This PR is very safe to merge as it fixes a clear validator bug without affecting simulation behavior
  2. The fix is simple, well-tested, and only affects validation logic, not the actual simulation mechanics
  3. Key files to review:
    • tidy3d/components/simulation.py
    • test_simulation.py (test updates)

2 files reviewed, no comments
Edit PR Review Bot Settings | Greptile

@jewettaijfc jewettaijfc self-assigned this Jul 18, 2025
@jewettaijfc jewettaijfc added the Bug something isnt working label Jul 18, 2025
@jewettaijfc jewettaijfc linked an issue Jul 18, 2025 that may be closed by this pull request
@jewettaijfc jewettaijfc changed the title _validate_boundary_spec_symmetry() now ignores 'name' attribute Giving opposite boundaries different names no longer causes a symmetry validator failure Jul 18, 2025
@jewettaijfc jewettaijfc force-pushed the jewettaijfc/issue2635 branch from c969a44 to 6c3eaa1 Compare July 18, 2025 20:28
@jewettaijfc jewettaijfc marked this pull request as ready for review July 18, 2025 20:34
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Reviewing changes made in this pull request

Copy link
Contributor

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

  • tidy3d/components/simulation.py (100%)

Summary

  • Total: 5 lines
  • Missing: 0 lines
  • Coverage: 100%

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

Thanks @jewettaijfc looks great! Just a minor nitpick.

@jewettaijfc jewettaijfc force-pushed the jewettaijfc/issue2635 branch from b7bffdd to ea07ccc Compare July 21, 2025 15:12
Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a comment

Choose a reason for hiding this comment

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

LGTM!

@yaugenst-flex yaugenst-flex enabled auto-merge July 21, 2025 15:28
@yaugenst-flex yaugenst-flex added this pull request to the merge queue Jul 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jul 21, 2025
@jewettaijfc jewettaijfc added this pull request to the merge queue Jul 21, 2025
@jewettaijfc jewettaijfc removed this pull request from the merge queue due to a manual request Jul 21, 2025
@jewettaijfc jewettaijfc added this pull request to the merge queue Jul 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Jul 21, 2025
@jewettaijfc jewettaijfc added this pull request to the merge queue Jul 21, 2025
Merged via the queue into develop with commit 60f9714 Jul 21, 2025
45 checks passed
@jewettaijfc jewettaijfc deleted the jewettaijfc/issue2635 branch July 21, 2025 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug something isnt working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A more complex check for boundary plus and minus
2 participants