-
Notifications
You must be signed in to change notification settings - Fork 473
Unmigrate the insolation utils #1211
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: v2.0-refactor
Are you sure you want to change the base?
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.
Greptile Overview
Greptile Summary
Moves insolation.py and zenith_angle.py from physicsnemo/nn/ back to physicsnemo/utils/. The rationale is sound - these modules only use numpy/pandas/datetime and have no torch dependencies, making them general utility functions rather than neural network components.
Critical Issues Found:
- Test file
test/nn/test_zenith_angle.pyimports from the oldphysicsnemo.nn.zenith_anglepath and will break - Compatibility layer in
physicsnemo/compat/__init__.pyhas backwards mappings that need to be reversed to point oldnn.*paths to newutils.*paths
Confidence Score: 1/5
- This PR will break existing tests and has incorrect compatibility mappings
- The file moves themselves are clean and well-justified, but two critical issues prevent this from being merge-ready: (1) test imports still point to the old location and will fail, and (2) the compatibility layer mappings are backwards and need to be inverted
- Pay attention to
test/nn/test_zenith_angle.py(broken import) andphysicsnemo/compat/__init__.py(backwards compat mappings)
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| physicsnemo/utils/insolation.py | 5/5 | File moved from nn/ to utils/ with no content changes - safe |
| physicsnemo/utils/zenith_angle.py | 5/5 | File moved from nn/ to utils/ with no content changes - safe |
Sequence Diagram
sequenceDiagram
participant PR as PR Author
participant nn as physicsnemo.nn
participant utils as physicsnemo.utils
participant compat as physicsnemo.compat
participant tests as test/nn/test_zenith_angle.py
Note over PR,tests: File Migration (v2.0-refactor)
PR->>nn: Remove insolation.py
PR->>nn: Remove zenith_angle.py
PR->>utils: Add insolation.py
PR->>utils: Add zenith_angle.py
Note over compat: ⚠️ OUTDATED: Still maps utils→nn
compat-->>nn: Points to OLD location
Note over tests: ⚠️ BROKEN: Imports from old path
tests-->>nn: Imports from physicsnemo.nn.zenith_angle
Additional Comments (2)
-
physicsnemo/utils/zenith_angle.py, line 1 (link)logic: the test file at
test/nn/test_zenith_angle.py:34still imports fromphysicsnemo.nn.zenith_angleand will break after this move -
physicsnemo/utils/insolation.py, line 1 (link)logic: the compat map at
physicsnemo/compat/__init__.py:65-66has backwards mappings - it mapsutils.insolation→nn.insolationandutils.zenith_angle→nn.zenith_angle, but after this PR the files ARE in utils, so the compat layer should map the OLDnn.*paths to the NEWutils.*paths
2 files reviewed, 2 comments
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.
Greptile Overview
Greptile Summary
Reverted the migration of insolation and zenith_angle modules from physicsnemo.nn back to physicsnemo.utils.
- Removed compatibility mappings in
COMPAT_MAPforphysicsnemo.utils.insolation→physicsnemo.nn.insolationandphysicsnemo.utils.zenith_angle→physicsnemo.nn.zenith_angle - Added comprehensive test file
test/utils/test_zenith_angle.pythat imports directly fromphysicsnemo.utils.zenith_angle - The modules are now located in their original
utils/directory, which is more appropriate since they are general-purpose utility functions rather than neural network components - No other code in the codebase imports from the
nnlocations, ensuring this change is safe
Confidence Score: 5/5
- This PR is safe to merge with no risk
- Simple, clean refactoring that moves modules back to their logical location. No code imports from the
nnpaths, and the test file correctly imports fromutils. The changes are minimal and well-contained. - No files require special attention
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| physicsnemo/compat/init.py | 5/5 | Removed deprecated compatibility mappings for insolation and zenith_angle from nn back to utils |
| test/utils/test_zenith_angle.py | 5/5 | Added comprehensive test file for zenith angle utilities with proper imports from physicsnemo.utils.zenith_angle |
Sequence Diagram
sequenceDiagram
participant User as User Code
participant Utils as physicsnemo.utils
participant Compat as physicsnemo.compat
participant NN as physicsnemo.nn
Note over Utils,NN: Before PR: v1 modules migrated to nn/
User->>NN: import zenith_angle/insolation
NN-->>User: Module (in nn/)
User->>Compat: Use old path (utils.zenith_angle)
Compat->>NN: Redirect to nn.zenith_angle
NN-->>User: Module
Note over Utils,NN: After PR: Unmigration back to utils/
User->>Utils: import zenith_angle/insolation
Utils-->>User: Module (in utils/)
Note over Compat: Compat mappings removed
Note over NN: No longer in nn/
2 files reviewed, no comments
PhysicsNeMo Pull Request
Description
Move the insolation and zenith angle back to
utilsfromnn. My arg for doing this is that they are not torch functions and are just more general utility functions.Checklist
Dependencies
Review Process
All PRs are reviewed by the PhysicsNeMo team before merging.
Depending on which files are changed, GitHub may automatically assign a maintainer for review.
We are also testing AI-based code review tools (e.g., Greptile), which may add automated comments with a confidence score.
This score reflects the AI’s assessment of merge readiness and is not a qualitative judgment of your work, nor is
it an indication that the PR will be accepted / rejected.
AI-generated feedback should be reviewed critically for usefulness.
You are not required to respond to every AI comment, but they are intended to help both authors and reviewers.
Please react to Greptile comments with 👍 or 👎 to provide feedback on their accuracy.