viz.spectogram refactor #486
Conversation
WalkthroughThis pull request introduces modifications to three files in the DASCore library. In Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
dascore/viz/spectrogram.py (5)
14-22: Add type hints to the_get_other_dimfunction for clarity and consistencyAdding type annotations to the function parameters and return value enhances readability and aids in static type checking.
Apply this diff to add type hints:
+from typing import Optional, Sequence def _get_other_dim(dim, dims): +def _get_other_dim(dim: str, dims: Sequence[str]) -> Optional[str]: if not isinstance(dim, str): raise TypeError(f"Expected 'dim' to be a string, got {type(dim).__name__}.") if dim not in dims: raise ValueError(f"The dimension '{dim}' is not in patch's dimensions {dims}.") if len(dims) == 1: return None else: return dims[0] if dims[1] == dim else dims[1]
47-47: Correct the typo: "spectogram" should be "spectrogram"In line 47, the word "spectogram" is misspelled; it should be "spectrogram".
Apply this diff to correct the typo:
- Dimension along which spectogram is being plotted. + Dimension along which spectrogram is being plotted.
51-51: Correct the typo: "caluclated" should be "calculated"In line 51, the word "caluclated" is misspelled; it should be "calculated".
Apply this diff to correct the typo:
- dimension is caluclated. No need to specify if other dimension's + dimension is calculated. No need to specify if other dimension's
75-75: Correct the typo: "spectogram" should be "spectrogram"In the error message, correct the misspelling of "spectogram" to "spectrogram".
Apply this diff to correct the typo:
- raise ValueError("Can only make spectogram of 1D or 2D patches.") + raise ValueError("Can only make spectrogram of 1D or 2D patches.")
87-88: Adjust punctuation in the error messageIn the error message, the period should be placed outside the quotation marks for consistency.
Apply this diff:
- f"The aggr_domain '{aggr_domain}' should be 'time' or 'frequency.'" + f"The aggr_domain '{aggr_domain}' should be 'time' or 'frequency'."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dascore/proc/correlate.py(0 hunks)dascore/viz/spectrogram.py(1 hunks)docs/index.qmd(0 hunks)tests/test_viz/test_spectrogram.py(2 hunks)
💤 Files with no reviewable changes (2)
- docs/index.qmd
- dascore/proc/correlate.py
⏰ 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_min_deps (windows-latest, 3.13)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
- GitHub Check: test_code (ubuntu-latest, 3.10)
🔇 Additional comments (1)
dascore/viz/spectrogram.py (1)
32-33: Verify type hint syntax compatibility with supported Python versionsThe use of the
|operator in type annotations (e.g.,float | Sequence[float] | None) requires Python 3.10 or higher. Please ensure that the codebase's minimum Python version supports this syntax.If the codebase supports earlier Python versions, consider using
Unionfrom thetypingmodule:+from typing import Union scale: float | Sequence[float] | None = None, - scale: float | Sequence[float] | None = None, + scale: Union[float, Sequence[float], None] = None, scale_type: Literal["relative", "absolute"] = "relative", - scale_type: Literal["relative", "absolute"] = "relative", + scale_type: Literal["relative", "absolute"] = "relative",✅ Verification successful
Union type syntax is compatible with project's Python version requirements
The codebase requires Python 3.10 or higher (specified in pyproject.toml), which fully supports the union type operator syntax. No changes needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check common Python project files for version requirements echo "=== Checking pyproject.toml ===" cat pyproject.toml 2>/dev/null || echo "File not found" echo -e "\n=== Checking setup.py ===" cat setup.py 2>/dev/null || echo "File not found" echo -e "\n=== Checking setup.cfg ===" cat setup.cfg 2>/dev/null || echo "File not found" echo -e "\n=== Checking README.md for Python version ===" rg -i "python.*version|requires.*python" README.md 2>/dev/null || echo "No Python version found in README.md" echo -e "\n=== Looking for other union type syntax usage ===" rg "\w+\s*:\s*\w+\s*\|" 2>/dev/null || echo "No union type syntax found"Length of output: 16902
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #486 +/- ##
==========================================
- Coverage 99.85% 99.85% -0.01%
==========================================
Files 118 118
Lines 9703 9693 -10
==========================================
- Hits 9689 9679 -10
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
dascore/viz/spectrogram.py (4)
14-22: LGTM! Consider adding a docstring.The helper function is well-structured with proper type checking and validation.
Add a docstring to improve documentation:
def _get_other_dim(dim, dims): + """ + Get the other dimension from a tuple of dimensions. + + Parameters + ---------- + dim : str + The dimension to find the complement of + dims : tuple + Tuple of dimension names + + Returns + ------- + str or None + The other dimension name, or None for 1D patches + + Raises + ------ + TypeError + If dim is not a string + ValueError + If dim is not in dims + """ if not isinstance(dim, str):
73-75: Fix typo in error message.There's a typo in the error message: "spectogram" should be "spectrogram".
- raise ValueError("Can only make spectogram of 1D or 2D patches.") + raise ValueError("Can only make spectrogram of 1D or 2D patches.")
42-46: Improve docstring foraxparameter.The description for the
axparameter could be more descriptive.ax - A matplotlib object, if None create one. + A matplotlib Axes object to plot on. If None, a new figure and axes will be created.
50-53: Fix typo in docstring.There's a typo in the docstring: "caluclated" should be "calculated".
"time" or "frequency" in which the mean value of the other - dimension is caluclated. No need to specify if other dimension's + dimension is calculated. No need to specify if other dimension's coord size is 1.tests/test_viz/test_spectrogram.py (1)
67-69: Consider adding more assertions.The tests verify the return type but could benefit from additional assertions:
test_1d_patch: Verify the dimensions of the resulting plottest_show: Verify that the plot is actually showndef test_1d_patch(self, random_patch): """Ensure spectrogram works with 1D patch.""" patch = random_patch.select(distance=0, samples=True).squeeze() axis = patch.viz.spectrogram(dim="time") assert isinstance(axis, plt.Axes) + # Verify the plot dimensions + assert len(axis.get_images()) == 1 + image = axis.get_images()[0] + assert len(image.get_array().shape) == 2 def test_show(self, random_patch, monkeypatch): """Ensure show path is callable.""" + show_called = False + def mock_show(): + nonlocal show_called + show_called = True - monkeypatch.setattr(plt, "show", lambda: None) + monkeypatch.setattr(plt, "show", mock_show) axis = random_patch.viz.spectrogram(dim="time", show=True) assert isinstance(axis, plt.Axes) + assert show_called, "plt.show() was not called"Also applies to: 74-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/viz/spectrogram.py(1 hunks)tests/test_viz/test_spectrogram.py(2 hunks)
⏰ 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_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code_min_deps (macos-latest, 3.12)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code_min_deps (ubuntu-latest, 3.12)
🔇 Additional comments (3)
dascore/viz/spectrogram.py (1)
77-93: LGTM! Well-structured implementation.The implementation is clean and follows a logical flow:
- Gets the other dimension using the helper function
- Handles aggregation based on the specified domain
- Returns the waterfall visualization with all parameters
tests/test_viz/test_spectrogram.py (2)
11-29: LGTM! Comprehensive test coverage.The tests for
_get_other_dimare well-structured and cover all edge cases:
- Valid dimension handling
- Invalid dimension validation
- Type checking
61-61: Fix typo in error message.There's a typo in the error message: "spectogram" should be "spectrogram".
- ValueError, match="Can only make spectogram of 1D or 2D patches" + ValueError, match="Can only make spectrogram of 1D or 2D patches"
ahmadtourei
added
ready_for_review
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dascore/viz/spectrogram.py (1)
51-51: Fix typo in docstring"caluclated" should be "calculated"
- dimension is caluclated. No need to specify if other dimension's + dimension is calculated. No need to specify if other dimension'stests/test_viz/test_spectrogram.py (1)
77-85: Consider enhancing edge case testsWhile the current tests cover basic functionality, consider adding assertions to verify:
- The shape of the output for 1D patches
- The content of the axes (labels, title)
- The return value matches input parameters (e.g., colormap)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/viz/spectrogram.py(1 hunks)tests/test_viz/test_spectrogram.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
🔇 Additional comments (5)
dascore/viz/spectrogram.py (3)
14-22: Well-structured helper function with robust error handling!The implementation is clean, handles edge cases well, and provides clear error messages.
27-36: Function signature aligns perfectly with PR objectives!The new parameters enable support for all waterfall arguments and provide flexibility for data aggregation in both time and frequency domains.
73-93: Clean implementation leveraging existing functionality!The refactored implementation efficiently uses existing functions while adding support for flexible aggregation domains.
tests/test_viz/test_spectrogram.py (2)
11-29: Comprehensive test coverage for the helper function!Tests thoroughly cover valid cases, invalid dimensions, and type validation.
41-73: Well-structured tests covering core functionality!Tests effectively validate the new parameters and error conditions.
|
@ahmadtourei, did you find code rabbit helpful here? I am trying to decide if it is useful or just annoying, but I haven't submitted a bigger PR since turning it on. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
dascore/viz/spectrogram.py (4)
14-22: Add type hints and docstring to the helper function.While the function implementation is solid with good error handling, it could benefit from:
- Type hints for parameters and return value
- A docstring explaining the purpose and behavior
Consider this improvement:
-def _get_other_dim(dim, dims): +def _get_other_dim(dim: str, dims: Sequence[str]) -> str | None: + """ + Get the other dimension from a patch's dimensions. + + Parameters + ---------- + dim : str + The primary dimension to check. + dims : Sequence[str] + The sequence of dimension names. + + Returns + ------- + str or None + The other dimension name, or None if dims has length 1. + + Raises + ------ + TypeError + If dim is not a string. + ValueError + If dim is not in dims. + """ if not isinstance(dim, str):
38-72: Add return value documentation.The docstring is comprehensive but missing the Returns section.
Add the following to the docstring:
**kwargs : dict, optional Passed to `scipy.signal.spectrogram` to control spectrogram options. See its documentation for options. + + Returns + ------- + matplotlib.axes.Axes + The matplotlib axis containing the spectrogram plot. """
74-76: Improve the error message for invalid dimensions.The error message could be more specific about the valid dimension count.
Consider this improvement:
- raise ValueError("Can only make spectrogram of 1D or 2D patches.") + raise ValueError(f"Expected patch with 1 or 2 dimensions, got {len(dims)} dimensions.")
78-94: LGTM! Clean and efficient implementation.The implementation effectively:
- Handles both 1D and 2D patches
- Supports flexible aggregation domains
- Reuses existing functionality
Consider defining valid aggregation domains as a constant to improve maintainability:
VALID_AGGR_DOMAINS = {"time", "frequency"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/viz/spectrogram.py(1 hunks)
⏰ 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 (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)
🔇 Additional comments (2)
dascore/viz/spectrogram.py (2)
5-7: LGTM! Clean import organization.The new imports from
collections.abcandtypingare appropriate for the type hints used in the function signature.
26-36: LGTM! Well-structured function signature with proper type hints.The new parameters enhance the function's flexibility while maintaining type safety.
I have not used it much. It caught a typo in this PR that was helpful. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dascore/viz/spectrogram.py (2)
14-22: LGTM! Well-structured helper function with robust validation.The helper function effectively validates inputs and handles edge cases. The error messages are clear and informative.
Consider adding type hints to improve code maintainability:
-def _get_other_dim(dim, dims): +def _get_other_dim(dim: str, dims: Sequence[str]) -> str | None:
92-94: Add runtime validation for scale_type.While the type hint uses
Literal["relative", "absolute"], there's no runtime validation to ensurescale_typecontains valid values. Consider adding validation before passing to waterfall.+ if scale_type not in ["relative", "absolute"]: + raise ValueError( + f"scale_type must be 'relative' or 'absolute', got '{scale_type}'" + ) return spec.viz.waterfall( ax=ax, cmap=cmap, scale=scale, scale_type=scale_type, log=log, show=show )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dascore/viz/spectrogram.py(1 hunks)
⏰ 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 (3)
dascore/viz/spectrogram.py (3)
27-72: LGTM! Comprehensive function signature with well-documented parameters.The changes align perfectly with the PR objectives:
- Support for waterfall arguments (scale, scale_type)
- Flexible aggregation domain control
- Clear parameter documentation
74-77: LGTM! Clear dimension validation.The validation of patch dimensions is explicit and provides a clear error message.
78-89: Consider consistent kwargs handling.The
kwargsparameter is only passed tospectrogram()in the 1D case (line 91) but not in the 2D aggregation cases (lines 82, 84). This inconsistency might lead to unexpected behavior.
Description
This PR enhances the
Patch.viz.spectogram()function and simply usesPatch.sepctogram()andPatch.viz.waterfall()functions under the hood so the new function supports all the arguments thatPatch.viz.waterfall()supports (e.g.,scale. The new function also gives users the option to aggregate the patch in time or frequency domain.Checklist
I have (if applicable):
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
segyiolibrary dependency.Tests