add type hints to patch functions #590
Conversation
WalkthroughAdds explicit type annotations to many public function signatures across processing and visualization modules, narrows detrend's type to a Literal, returns Python floats in an IO helper, centralizes trapezoid/trapz selection for integrate, and removes an inline comment from a CI workflow. No runtime/control-flow changes reported. Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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). (18)
🔇 Additional comments (3)
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
added
proc
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #590 +/- ##
=======================================
Coverage 99.92% 99.92%
=======================================
Files 129 129
Lines 11272 11274 +2
=======================================
+ Hits 11264 11266 +2
Misses 8 8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (1)
dascore/proc/detrend.py (1)
15-15: Consider renaming parameter to avoid shadowing built-in.The parameter name
typeshadows Python's built-intype()function. While this doesn't break functionality, consider renaming todetrend_typeormethodin a future refactor for better code hygiene.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
dascore/proc/coords.py(2 hunks)dascore/proc/correlate.py(2 hunks)dascore/proc/detrend.py(1 hunks)dascore/proc/filter.py(6 hunks)dascore/proc/hampel.py(1 hunks)dascore/proc/resample.py(2 hunks)dascore/proc/wiener.py(1 hunks)dascore/viz/waterfall.py(1 hunks)dascore/viz/wiggle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dascore/proc/detrend.py (1)
dascore/utils/patch.py (1)
patch_function(183-289)
dascore/proc/coords.py (2)
tests/conftest.py (1)
patch(379-381)dascore/core/patch.py (1)
coords(255-269)
⏰ 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). (18)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: Run benchmarks
🔇 Additional comments (16)
dascore/proc/detrend.py (1)
14-16: LGTM! Type hints correctly applied.The type annotations are accurate and the use of
Literalappropriately restricts thetypeparameter to valid values.dascore/proc/hampel.py (1)
98-105: LGTM! Type hints correctly applied.The explicit type annotations for
samples,approximate, and the return type are accurate and improve code clarity without altering behavior.dascore/viz/wiggle.py (1)
54-63: LGTM! Type hints correctly applied.All parameter type annotations are accurate and consistent with the function's documented behavior and implementation.
dascore/proc/wiener.py (1)
15-21: LGTM! Type hints correctly applied.The type annotations are accurate. The use of
float | Noneunion syntax requires Python 3.10+, which should be fine if that's already the project's minimum version.dascore/proc/coords.py (2)
24-25: LGTM! Type hints correctly applied.The type annotations for
reverseparameter and return type are accurate.
61-62: LGTM! Type hints correctly applied.The type annotations for
reverseparameter and return type are accurate.dascore/proc/correlate.py (2)
38-40: LGTM! Type hints correctly applied.The type annotations for
dim,undo_weighting, and return type are accurate and improve code clarity.
90-95: LGTM! Type hints correctly applied.The type annotation for
samplesparameter is accurate.dascore/viz/waterfall.py (1)
135-144: LGTM! Type hints correctly applied.The type annotation for
cmapparameter is accurate and consistent with matplotlib's API.dascore/proc/resample.py (2)
40-45: LGTM! Type hints correctly applied.The type annotation for
copyparameter is accurate.
157-163: LGTM! Type hints correctly applied.The type annotations for
interp_kind,samples, and return type are accurate. Thewindowparameter remains untyped, which is acceptable for this focused type-hinting PR.dascore/proc/filter.py (5)
98-100: LGTM!The type hints accurately reflect the function signature and match the implementation.
206-212: LGTM!The type hints accurately reflect the function signature. The
cvaltype hint inconsistency is already noted in thesobel_filterreview.
262-262: LGTM!The type hints accurately reflect the function signature and match the implementation.
330-337: LGTM!The type hints accurately reflect the function signature. The
cvaltype hint inconsistency is already noted in thesobel_filterreview.
397-404: LGTM!The type hints accurately reflect the function signature. The
cvaltype hint inconsistency is already noted in thesobel_filterreview.
CodSpeed Performance ReportMerging #590 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dascore/proc/filter.py (1)
210-210: Consider unifyingcvaltype hints across filter functions for consistency.The
cvalparameter is typed asfloatinmedian_filter(line 210),savgol_filter(line 335), andgaussian_filter(line 401), whilesobel_filterusesfloat | int(line 152). Since the underlying scipy functions (scipy.ndimage.median_filter,scipy.signal.savgol_filter, andscipy.ndimage.gaussian_filter) all accept numeric types, consider updating these type hints tofloat | intfor consistency and accuracy.🔎 Proposed fix for consistency
Update the type hints in the three functions:
def median_filter( patch: PatchType, samples: bool = False, mode: str = "reflect", - cval: float = 0.0, + cval: float | int = 0.0, **kwargs, ) -> PatchType:def savgol_filter( patch: PatchType, polyorder: int, samples: bool = False, mode: str = "interp", - cval: float = 0.0, + cval: float | int = 0.0, **kwargs, ) -> PatchType:def gaussian_filter( patch: PatchType, samples: bool = False, mode: str = "reflect", - cval: float = 0.0, + cval: float | int = 0.0, truncate: float = 4.0, **kwargs, ) -> PatchType:Also applies to: 335-335, 401-401
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/proc/filter.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dascore/proc/filter.py (1)
tests/conftest.py (1)
patch(379-381)
⏰ 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). (18)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: Run benchmarks
🔇 Additional comments (3)
dascore/proc/filter.py (3)
98-100: LGTM! Type hints are accurate.The type hints correctly reflect the parameter types and return type.
151-153: LGTM! Past review comment addressed.The
cvaltype hint now correctly reflects the runtime validation that accepts bothfloat | int.
262-262: LGTM! Type hints are accurate.The type hints correctly reflect the parameter types and return type.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/profile.yml(1 hunks)
⏰ 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). (18)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: Run benchmarks
| mode: instrumentation | ||
| run: ./.github/test_code.sh profile | ||
| token: ${{ secrets.CODSPEED_TOKEN }} # Optional for public repos | ||
| token: ${{ secrets.CODSPEED_TOKEN }} |
There was a problem hiding this comment.
Restore the removed comment or revert this change.
The removal of the "Optional for public repos" comment reduces documentation clarity about when the CODSPEED_TOKEN is required. This change is also unrelated to the PR's stated objective of adding type hints.
🔎 Proposed fix
- token: ${{ secrets.CODSPEED_TOKEN }}
+ token: ${{ secrets.CODSPEED_TOKEN }} # Optional for public repos📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| token: ${{ secrets.CODSPEED_TOKEN }} | |
| token: ${{ secrets.CODSPEED_TOKEN }} # Optional for public repos |
🤖 Prompt for AI Agents
.github/workflows/profile.yml around line 39: the inline comment "Optional for
public repos" that clarified when CODSPEED_TOKEN is required was removed,
reducing clarity; restore that comment immediately above or beside the token
line (token: ${{ secrets.CODSPEED_TOKEN }}) or revert the commit that removed it
so the workflow again documents that the secret is optional for public
repositories; keep wording consistent with repository style.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/io/febus/utils.py(1 hunks)dascore/utils/misc.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
dascore/utils/misc.py (3)
dascore/core/coordmanager.py (2)
shape(798-804)size(807-809)dascore/core/patch.py (2)
shape(286-299)size(302-315)dascore/core/coords.py (1)
size(473-475)
dascore/io/febus/utils.py (1)
dascore/utils/misc.py (1)
_maybe_unpack(463-469)
⏰ 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). (18)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: Run benchmarks
🔇 Additional comments (1)
dascore/io/febus/utils.py (1)
30-57: Usage of updated_maybe_unpackis correct.The function now uses
_maybe_unpackwithnp.mean(d_time). Sincenp.mean()returns a 0-dimensional numpy array (withshape=()), the updated_maybe_unpackwill return it unchanged rather than attempting to unpack it.This is the correct behavior given the bug fix in
_maybe_unpack. The return value is now a 0-d numpy array instead of a Python scalar, which should be compatible with downstream arithmetic operations (lines 75, 158 useblock_time).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dascore/transform/integrate.py (1)
48-49: Clearer numpy compatibility check.The two-step approach using
hasattrfollowed bygetattris more explicit and readable than the previous fallback logic.However, since the numpy version doesn't change during execution, consider moving this check outside the loop for efficiency:
🔎 Optional refactor to move version check outside loop
Move the check to module level or before the loop in
_get_definite_integral:+# At module level (after imports) +_TRAP_FUNC = getattr(np, "trapezoid" if hasattr(np, "trapezoid") else "trapz") + def _get_definite_integral(patch, dxs_or_vals, dims, axes): """Get a definite integral along axes.""" # ... existing code ... array = patch.data ndims = len(patch.shape) for dxs_or_val, ax in zip(dxs_or_vals, axes): - # Numpy 2/3 compat code - trap_name = "trapezoid" if hasattr(np, "trapezoid") else "trapz" - trap = getattr(np, trap_name) indexer = broadcast_for_index(ndims, ax, None, fill=slice(None)) if is_array(dxs_or_val): - array = trap(array, x=dxs_or_val, axis=ax)[indexer] + array = _TRAP_FUNC(array, x=dxs_or_val, axis=ax)[indexer] else: - array = trap(array, dx=dxs_or_val, axis=ax)[indexer] + array = _TRAP_FUNC(array, dx=dxs_or_val, axis=ax)[indexer]
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/profile.ymldascore/transform/integrate.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/profile.yml
⏰ 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). (18)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: Run benchmarks
Description
This PR just adds some missing type hints to patch functions.
Checklist
I have (if applicable):
Summary by CodeRabbit
Refactor
Behavior
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.