fix_correlate_shift_coord_length #485
Conversation
WalkthroughThis pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (15)
🔇 Additional comments (3)
Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #485 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 118 118
Lines 9702 9703 +1
=======================================
+ Hits 9688 9689 +1
Misses 14 14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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/correlate.py (1)
76-77: LGTM! The fix correctly handles odd-length coordinates.The added condition ensures that the new coordinate range maintains the same length as the input coordinate array by adding an extra step when the length is odd.
Consider adding docstring examples demonstrating both odd and even length cases to help users understand the behavior:
>>> # Example with odd length >>> patch = dc.get_example_patch(shape=(2, 11)) >>> dft = patch.dft("time") >>> dft_sq = dft * dft.conj() >>> idft = dft_sq.idft() >>> auto_patch = idft.correlate_shift(dim="time") >>> >>> # Example with even length >>> patch = dc.get_example_patch(shape=(2, 10)) >>> # ... same steps as above
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dascore/proc/correlate.py(1 hunks)docs/recipes/how_to_contribute.qmd(0 hunks)tests/test_proc/test_correlate.py(2 hunks)
💤 Files with no reviewable changes (1)
- docs/recipes/how_to_contribute.qmd
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.12)
- 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: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- 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: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
🔇 Additional comments (1)
tests/test_proc/test_correlate.py (1)
15-25: LGTM! Well-structured fixtures for testing both cases.The fixtures provide good test data with explicit odd (11) and even (10) lengths.
| def test_auto_correlation_odd_coord(self, random_patch_odd): | ||
| """Ensure correlate_shift works when dim's coord length is odd.""" | ||
| dft = random_patch_odd.dft(dim="time") | ||
| dft_conj = dft.conj() | ||
| dft_sq = dft * dft_conj | ||
| idft = dft_sq.idft() | ||
| assert isinstance(idft.correlate_shift(dim="time"), dc.Patch) | ||
|
|
||
| def test_auto_correlation_even_coord(self, random_patch_even): | ||
| """Ensure correlate_shift works when dim's coord length is even.""" | ||
| dft = random_patch_even.dft(dim="time") | ||
| dft_conj = dft.conj() | ||
| dft_sq = dft * dft_conj | ||
| idft = dft_sq.idft() | ||
| assert isinstance(idft.correlate_shift(dim="time"), dc.Patch) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen the test assertions.
While testing instance type is good, consider adding assertions to verify:
- The coordinate lengths match the input
- The correlation results are as expected
Add these assertions to both test methods:
def test_auto_correlation_odd_coord(self, random_patch_odd):
"""Ensure correlate_shift works when dim's coord length is odd."""
dft = random_patch_odd.dft(dim="time")
dft_conj = dft.conj()
dft_sq = dft * dft_conj
idft = dft_sq.idft()
- assert isinstance(idft.correlate_shift(dim="time"), dc.Patch)
+ result = idft.correlate_shift(dim="time")
+ assert isinstance(result, dc.Patch)
+ # Verify coordinate lengths match
+ assert len(result.get_coord("lag_time")) == len(random_patch_odd.get_coord("time"))
+ # Verify maximum correlation at zero lag
+ assert np.argmax(result.data) == len(result.get_coord("lag_time")) // 2Apply similar changes to test_auto_correlation_even_coord.
Committable suggestion skipped: line range outside the PR's diff.
ahmadtourei
added
ready_for_review
|
Hey @ahmadtourei, I am not sure why the ci is failing, I will look into it today or tomorrow. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/run_min_dep_tests.yml (1)
Line range hint
1-57: Consider adjusting the timeout for minimum dependency tests.The current timeout of 25 minutes might need adjustment when running with minimum dependencies, as package resolution and installation might take longer.
Consider increasing the
timeout-minutesvalue or monitoring job execution times to ensure it's sufficient:- timeout-minutes: 25 + timeout-minutes: 30🧰 Tools
🪛 actionlint (1.7.4)
47-47: shellcheck reported issue in this script: SC2102:info:1:17: Ranges can only match single chars (mentioned due to duplicates)
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/run_min_dep_tests.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/run_min_dep_tests.yml
47-47: shellcheck reported issue in this script: SC2102:info:1:17: Ranges can only match single chars (mentioned due to duplicates)
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.10)
- 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: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
🔇 Additional comments (2)
.github/workflows/run_min_dep_tests.yml (2)
45-47: LGTM! Package installation step looks good.The change to use pip for installation with test dependencies is appropriate and follows standard practices.
🧰 Tools
🪛 actionlint (1.7.4)
47-47: shellcheck reported issue in this script: SC2102:info:1:17: Ranges can only match single chars (mentioned due to duplicates)
(shellcheck)
51-52: Verify the shell configuration and test script.The shell configuration has been updated to use
bash -el {0}which is good for error handling, but we should verify the test script exists and has proper permissions.Run this script to verify the test configuration:
Also applies to: 56-57
✅ Verification successful
Shell configuration and test script verification successful
The test script exists with proper executable permissions (755) and passes basic syntax checks. There's a minor ShellCheck suggestion about quoting variables, but it doesn't impact the core functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test script existence and permissions # Check if test script exists and is executable if [ -f ./.github/test_code.sh ]; then echo "Test script exists" ls -l ./.github/test_code.sh else echo "Test script not found" exit 1 fi # Check shell script syntax bash -n ./.github/test_code.sh # Check for common shell script issues if command -v shellcheck &> /dev/null; then shellcheck ./.github/test_code.sh fiLength of output: 604
d-chambers
left a comment
d-chambers
left a comment
There was a problem hiding this comment.
Looking over this, I think we forgot to propagate the units.
We might modify this slightly to remove the if clause and handle the units like so:
_new_coord = dc.get_coord(start=new_start, stop=new_end, step=step, units=coord.units)
new_coord = _new_coord.change_length(len(coord))Up to you though.
|
For the record, I am not sure why uv was seg faulting but I fixed the minimum dependency tests by simply using pip to install dascore and then it worked fine. |
… into fix_correlate_coord_issue
Description
This PR fixes a bug in the
Patch.correlate_shift()function. With the previous version of this function, there was an AssertionError as mentioned below:len(coord)is 99 whilelen(new_coord)is 98.I'm not sure why other random patches with even time coord length, such as
(5, 40)or(5, 70), did not raise errors though.Checklist
I have (if applicable):
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation