Skip to content

Change coordinates when Charge simulations aren't in the xy plane #2610

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

Closed
wants to merge 5 commits into from

Conversation

marc-flex
Copy link
Contributor

@marc-flex marc-flex commented Jun 27, 2025

Greptile Summary

Added coordinate transformation functionality to rotate 2D charge simulations from z-plane to y-plane orientation with comprehensive handling of structure components and validation.

  • Added _create_charge_copy_xy() method in tidy3d/components/tcad/simulation/heat_charge.py to enable z-plane to y-plane rotation of 2D charge simulations
  • Added validation to prevent rotation of 1D charge simulations in _create_charge_copy_xy()
  • Implemented coordinate transformations for semiconductor mediums, doping boxes, boundary conditions, and monitors during rotation
  • Added comprehensive test case test_charge_copy_xy in tests/test_components/test_heat_charge.py to verify rotation functionality

@marc-flex marc-flex self-assigned this Jun 27, 2025
@marc-flex marc-flex force-pushed the marc/rotation_devsim branch from 958c1f8 to 384cff4 Compare July 7, 2025 17:21
@marc-flex marc-flex force-pushed the marc/rotation_devsim branch from 586040b to c3d5226 Compare July 8, 2025 14:10
@marc-flex marc-flex requested review from momchil-flex and dbochkov-flexcompute and removed request for momchil-flex July 8, 2025 14:16
@marc-flex marc-flex marked this pull request as ready for review July 8, 2025 14:16
@marc-flex marc-flex changed the title Adding a rotation function Change coordinates when Charge simulations aren't in the xy plane Jul 8, 2025
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.

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

assert list(changed_sim.structures[0].medium.charge.N_d.coords["z"].data) == [0]

assert changed_sim.structures[0].medium.charge.N_a[0].source == "ymin"
assert changed_sim.structures[0].medium.charge.N_a[0].size == (1, 2, 1)
Copy link

Choose a reason for hiding this comment

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

logic: The assumption that the source should be 'ymin' is not explicitly tested. Add assertion for other source values ('ymax', 'xmin', 'xmax') to ensure complete rotation handling.

Copy link
Contributor

github-actions bot commented Jul 8, 2025

Diff Coverage

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

  • tidy3d/components/tcad/simulation/heat_charge.py (92.9%): Missing lines 1796,1801,1804,1857,1865

Summary

  • Total: 70 lines
  • Missing: 5 lines
  • Coverage: 92%

tidy3d/components/tcad/simulation/heat_charge.py

  1792 
  1793         sim_types = self._get_simulation_types()
  1794 
  1795         if TCADAnalysisTypes.CHARGE not in sim_types:
! 1796             return self
  1797 
  1798         zero_dims = self.zero_dims
  1799 
  1800         if len(zero_dims) > 1:
! 1801             raise SetupError("Charge simulation can only be 2- or 3-D")
  1802 
  1803         if len(zero_dims) == 0 or zero_dims[0] == 2:
! 1804             return self
  1805 
  1806         def _update_center_size(
  1807             zero_dims: list[int], center: list[float], size: list[float]
  1808         ) -> tuple[list[float], list[float]]:

  1853                                         center=new_center, size=new_size, source=source
  1854                                     )
  1855                                 )
  1856                             else:
! 1857                                 new_boxes.append(doping_box)
  1858                             sc_medium = sc_medium.updated_copy(**{key: new_boxes})
  1859                 struct_medium[structure] = structure.medium.updated_copy(charge=sc_medium)
  1860 
  1861         # change structures

  1861         # change structures
  1862         new_structures = []
  1863         for structure in self.structures:
  1864             if not isinstance(structure.geometry, Box):
! 1865                 raise SetupError(
  1866                     "2D Charge simulations defined in planes other than xy can only use Box geometries, "
  1867                 )
  1868             else:
  1869                 new_center, new_size = _update_center_size(

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

it seems like we don't really need to put this in the frontend? Just thinking that if we need to fix anything later, we would need make changes to frontend instead of just backend

@marc-flex
Copy link
Contributor Author

it seems like we don't really need to put this in the frontend? Just thinking that if we need to fix anything later, we would need make changes to frontend instead of just backend

I think you're right. I'll do all modifications in this PR and then I'll think how to move this to backend

@momchil-flex momchil-flex added the 2.9 will go into version 2.9.* label Jul 16, 2025
@marc-flex
Copy link
Contributor Author

Closing this PR since coordinate transformation is dealt with entirely in the back end

@marc-flex marc-flex closed this Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.9 will go into version 2.9.* Charge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants