-
Notifications
You must be signed in to change notification settings - Fork 67
fix(tidy3d): FXC-4644-tidy-3-d-client-add-support-for-num-py-2-4-0 #3119
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
base: develop
Are you sure you want to change the base?
fix(tidy3d): FXC-4644-tidy-3-d-client-add-support-for-num-py-2-4-0 #3119
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (1)
-
tests/test_components/test_mode_interp.py, line 627-628 (link)logic: Remove temporary debugging print statements
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Context Used: Rule from
dashboard- Remove temporary debugging code (print() calls), commented-out code, and other workarounds before fi... (source)
6 files reviewed, 1 comment
a0cbfdf to
94cef22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Comments (2)
6 files reviewed, 2 comments
94cef22 to
bdd890c
Compare
bdd890c to
a463d68
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/base.pyLines 206-214 206
207 @pydantic.root_validator(pre=True)
208 def _coerce_numpy_scalars(cls, values: dict[str, Any]) -> dict[str, Any]:
209 if not isinstance(values, dict):
! 210 return values
211
212 for name, field in cls.__fields__.items():
213 if name not in values or not cls._field_allows_scalar(field):
214 continue |
Greptile Summary
Added support for NumPy 2.4.0 by addressing breaking changes in scalar handling, array indexing, and deprecated functions.
Key Changes:
Tidy3dBaseModelvia root validator to convertnp.genericand 0-d arrays to Python scalarsnp.sum()with built-insum()for generator expressions to avoid numpy scalar issuesnp.argwhere()calls to explicitly extract first column ([:, 0]) since numpy 2.4.0 always returns 2D arrays.item()calls beforefloat()/complex()conversions of xarray coordinatesnp.trapezoid(numpy ≥2.0) vsnp.trapz(numpy <2.0)Callabletocollections.abcandSelftotypingmoduleIssues Found:
tidy3d/components/data/sim_data.py:649andtidy3d/components/tcad/data/sim_data.py:436) still usefloat(field_data.coords[...])without.item()and will fail with numpy 2.4.0Confidence Score: 3/5
sim_data.pyfiles that use the same pattern (float(coords[...])) fixed elsewhere. These will cause runtime failures with numpy 2.4.0, making the compatibility incomplete.tidy3d/components/data/sim_data.pyandtidy3d/components/tcad/data/sim_data.py- both have missing.item()calls that will cause runtime failuresImportant Files Changed
Callableimport tocollections.abcandSelftotyping[:, 0]indexing to extract first column fromnp.argwhereresult for numpy 2.4.0 compatibility.item()call when converting xarray coordinate to float - will fail with numpy 2.4.0.item()call when converting xarray coordinate to float - will fail with numpy 2.4.0Sequence Diagram
sequenceDiagram participant User as User Code participant Base as Tidy3dBaseModel participant Geom as GeometryGroup participant Utils as geometry/utils participant XArray as xarray Coords participant NumPy as NumPy 2.4.0 Note over NumPy: NumPy 2.4.0 changes:<br/>- Scalars no longer cast implicitly<br/>- argwhere returns 2D array<br/>- trapz renamed to trapezoid User->>Base: Create model with numpy scalar Base->>Base: _coerce_numpy_scalars (root validator) Base->>NumPy: Check if np.generic or 0-d array NumPy-->>Base: Return numpy scalar type Base->>Base: Call .item() to extract Python scalar Base-->>User: Model created with Python scalars User->>Geom: Calculate volume/surface_area Geom->>Geom: Use sum() instead of np.sum() Note over Geom: Built-in sum() works with<br/>generators, avoids numpy issues Geom-->>User: Return calculated value User->>Utils: _shift_value_signed Utils->>NumPy: np.argwhere(condition) NumPy-->>Utils: 2D array [[idx], ...] Utils->>Utils: Extract [:, 0] to get 1D array Utils-->>User: Return correct indices User->>XArray: Access xarray coordinate XArray-->>User: Returns numpy scalar (2.4.0) User->>User: Call .item() before float()/complex() User-->>User: Correctly converted to Python type