Skip to content

✨ FEAT: Add reflection transformation with verification tests #2414

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Conversation

damianofranzo
Copy link

This PR addresses feature request #2157 by adding the capability to apply reflection transformations to geometry objects.

Key changes include:

  • Implementation of reflection logic via AbstractReflection and ReflectionFromPlane classes, calculating the reflection matrix from a normal vector with input validation.
  • Addition of the method Transformed.reflection(normal) to provide the 4x4 reflection matrix.
  • Addition of the method .reflected(normal) to the class Geometry to return the reflected copy of the geometry.
  • Addition of test_general_reflection to verify the reflection transformation is invariant to the scale and sign of the normal vector.
  • Addition of other tests to verify the correct reflection of several geometries.

Includes necessary imports and type definitions.

Considerations

  • The rotation matrix is simply derived from the definition from Wikipedia: $R = I - 2 \mathbf{\hat{n}} \mathbf{\hat{n}}^T $, where $\mathbf{\hat{n}}$ is the normal vector of the reflection plane. The input normal vector is automatically normalized internally. An error is raised only if the input normal's magnitude is close to zero.
  • The class AbstractReflection is basically a copy of AbstractRotation, with the difference that a reflection can never be an identity, and as a consequence, isidentity is omitted.
  • The class ReflectionFromPlane only accepts a normal in the format (x, y, z). Initially, I thought the input could also have been 0, 1, or 2 to specify YZ, XZ, and XY planes, respectively, but then I realized that this could have caused some confusion when used as API.
  • This PR focuses solely on adding reflection and is conceived as atomic. Therefore, it avoided any kind of refactoring (let's say, merging AbstractReflection and AbstractRotation into a more general abstraction, adding a possible center of reflection/rotation, or operations of this type).
  • The testing covers sufficient cases, but let me know if you want me to extend it to additional scenarios.

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.

Thanks! This is looking good!

Just one thing I'm wondering: have you checked the codebase for places where transformations are used and where the reflection might also need to be incorportaed?

@damianofranzo damianofranzo force-pushed the damianofranzo/reflection_transform branch from f8f4fce to ca052da Compare April 26, 2025 09:44
@damianofranzo
Copy link
Author

@momchil-flex Thank you!

The geometrical transformations are mainly checked in test_geometry.py, so I incorporated the changes into that file. In particular, I added reflected or reflection to

  • test_planar_transform to ensure that the transformed geometry does not project outside its defining plane
  • test_transforms to ensure that the reflection works under some controlled scenarios.
  • test_general_reflection to check fundamental properties of the reflection matrix

Regarding other uses of geometrical transformations:

  • test_structure.py and test_autograd.py. I noticed Polyslab has a custom rotated method, and I suppose I could benefit from having its own reflected method. I will have a look at it.
  • test_medium.py. I saw some usage of rotationAroundAxis in it, but it seems that it was mainly used for dealing with anisotropic properties, and I did not feel the need to add reflection there.
  • mode_solver.py. I found the part you referred to where you do translation, rotation, and inverse translation. Since this is part of a plugin, I suppose further discussion is needed before adding new APIs.

Let me know what you think.

@damianofranzo
Copy link
Author

damianofranzo commented Apr 26, 2025

I implemented the specialized reflected method for PolySlab with additional tests covering its usage.

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.

Nice, indeed, from the things you identified, materializing the transformation immediately for the PolySlab for efficiency seems the only thing that needed to be done, thanks for getting it in already!

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.

Thanks @damianofranzo this basically looks good for me to integrate into our codebase, awesome job.

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Added
- For efficiency, `reflected` `PolySlab`-s now return updated `PolySlab` objects rather than a `Transformed` object, except when the normal of the plane of reflection has a non-zero component along the slab axis, in which case `Transformed` is still returned.
- The method `Geometry.reflected` can be used to create a reflected copy of any geometry off a plane.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is very nitpicky but I would just merge these two items into one because no "change" was made to the reflected operation as it didn't exist before. Maybe something like

"The method Geometry.reflected can be used to create a reflected copy of any geometry off a plane. As for other transformations, for efficiency, reflected PolySlab-s directly return an updated PolySlab objects rather than a Transformed object, except when the normal of the plane of reflection has a non-zero component along the slab axis, in which case Transformed is still returned."

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants