Conversation
…d TimeDriver classes This commit enhances the `Experiment` and `TimeDriver` classes by updating type annotations and improving docstrings for clarity. The `hydra_config` property in `Experiment` now returns a `HydraConf` object, and the `end_at` setter in `TimeDriver` has been refactored to normalize the end time input. Additionally, the `array_cells` method in `PatchModule` has been updated to return a numpy array, and the `merge_parameters` function in `args.py` now uses `cast` for better type safety. These changes aim to improve code readability and maintainability while ensuring compliance with typing standards.
…e class This commit updates the `BaseNature` class to check for the existence of a module in the `layers` before adding it. This change prevents duplicate layers from being added, enhancing the integrity of the layer management process. The code now ensures that only unique modules are added, improving overall functionality and maintainability.
WalkthroughThe PR tightens type annotations and config typing, changes time-step handling to apply deltas, normalizes end-time handling with strict types, prevents duplicate spatial layers, adjusts patch array/iterator types, and returns Matplotlib Path objects for built-in markers. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Model.run_model
participant Model as Model
participant Time as TimeDriver
Note over Runner,Model: Old flow (before)
Runner->>Model: for each step -> call model.step()
Model->>Time: Time.go() -- (advance every iteration)
Model->>Model: update state
Note over Runner,Model: New flow (after)
Runner->>Model: set steps = N (or increment)
Model->>Time: compute delta = steps - prev_steps
alt delta > 0
Model->>Time: Time.go(delta) -- (advance by delta)
else delta <= 0
Note right of Time: no advancement
end
Runner->>Model: for each step -> call model.step() -- no automatic Time.go()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
abses/core/experiment.py(2 hunks)abses/core/model.py(1 hunks)abses/core/time_driver.py(2 hunks)abses/space/nature.py(1 hunks)abses/space/patch.py(3 hunks)abses/utils/args.py(2 hunks)abses/viz/customize_marker.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursorrules)
**/*.py: For any Python file, add typing annotations to every function and class, including explicit return types (use None where appropriate)
Add descriptive docstrings to all Python functions and classes; follow PEP 257 conventions and update existing docstrings as needed
Files:
abses/core/experiment.pyabses/core/time_driver.pyabses/utils/args.pyabses/space/nature.pyabses/viz/customize_marker.pyabses/core/model.pyabses/space/patch.py
🧬 Code graph analysis (2)
abses/core/time_driver.py (1)
abses/utils/time.py (2)
is_positive_int(25-30)parse_datetime(33-60)
abses/core/model.py (4)
abses/core/base_module.py (1)
time(189-195)abses/core/base_variable.py (1)
time(129-135)abses/core/protocols.py (2)
time(256-256)go(69-69)abses/core/time_driver.py (1)
go(234-238)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Tests 3.12 on windows-latest
- GitHub Check: Tests 3.11 on windows-latest
- GitHub Check: Tests 3.11 on macos-latest
- GitHub Check: Tests 3.13 on windows-latest
- GitHub Check: Tests 3.13 on macos-latest
- GitHub Check: Tests 3.12 on ubuntu-latest
- GitHub Check: Tests 3.13 on ubuntu-latest
- GitHub Check: Tests 3.12 on macos-latest
- GitHub Check: Tests 3.11 on ubuntu-latest
- GitHub Check: nbmake
- GitHub Check: nbmake
🔇 Additional comments (11)
abses/space/nature.py (1)
85-86: LGTM! Duplicate prevention improves robustness.The conditional check prevents duplicate layer additions when a module is re-created or re-added, which is a solid defensive practice.
abses/utils/args.py (2)
8-8: Type import added for explicit casting.The
castimport supports the explicit type casting added at line 48.
48-48: Explicit cast improves type checker confidence.While the function signature already declares
DictConfigas the return type, the explicitcasthelps type checkers verify thatmergedis indeed aDictConfigat this point, reducing false positives in static analysis.abses/space/patch.py (3)
372-377: LGTM! Type annotation and documentation clarified.The return type has been simplified to
np.ndarray, and the docstring now explicitly describes the return value as a 2D object-dtype numpy array containingPatchCellinstances. This improves clarity.
444-444: LGTM! Return type widened to match actual behavior.The return type now correctly reflects that the method can return either
np.ndarrayorxr.DataArraydepending on thedtypeparameter.
657-662: LGTM! Manual iteration provides explicit typing.The implementation now manually iterates over indices, making the coordinate yielding behavior more explicit and easier to type-check.
abses/core/experiment.py (2)
45-45: Import updated for improved type precision.The addition of
HydraConfsupports the more precise return type annotation at line 211.
211-212: LGTM! Return type matches Hydra's actual type.The return type has been updated from
DictConfigtoHydraConf, which more accurately reflects whatHydraConfig.get()returns. This improves type safety.abses/viz/customize_marker.py (1)
27-34: LGTM! Standardized return type improves consistency.The function now consistently returns a
Pathobject for both built-in matplotlib markers and Font Awesome icons, rather than returning a string for built-in markers. This standardization improves type safety and API consistency.Note: This is a breaking change if any callers expected string symbols for built-in markers.
abses/core/time_driver.py (2)
126-127: Private attribute added for normalized end time.The
_end_dtattribute stores the normalized end time value, supporting the refactored setter logic.
337-355: LGTM! Robust end-time normalization with strict typing.The setter now explicitly normalizes inputs to
DateTime | int | None:
NoneremainsNone- Positive integers are preserved as
int- Strings/datetimes are converted to timezone-naive
DateTimeobjects- Invalid types raise
TypeErrorThis approach provides clear type guarantees and explicit error handling, improving maintainability and debuggability.
This commit updates the `MainModel` class to enhance type checking for the `steps` parameter. The previous check for `steps` being an integer has been replaced with a check on the `delta` value, ensuring that the logic correctly validates the number of steps before advancing time. This change improves error handling and maintains the integrity of the model's state management.
SongshGeo
changed the title
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements