Mute #579
Mute #579
Conversation
WalkthroughAdds line- and slope-based muting functions, a full() fill operation, exposes these proc functions as Patch class aliases, deprecates slope_filter's notch parameter in favor of invert, adds a 2D line-intersection utility and boxcar window alias, and includes tests and benchmarks for the new features. Changes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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
patch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #579 +/- ##
========================================
Coverage 99.92% 99.92%
========================================
Files 128 129 +1
Lines 10878 11153 +275
========================================
+ Hits 10870 11145 +275
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dascore/proc/filter.py (1)
581-586: Fix NameError in UnitError message (undefineddims).
_maybe_transform_unitsformats an error string withdims, but that symbol is not in scope. If this branch executes, it will raise a NameError instead of UnitError. Replace withfreq_dims(available in scope) or pass the originaldimsin.Apply this diff:
- msg = ( - f"Units of {units} specified in Patch.slope_filter, but units " - f"are not defined for both specified dimensions: {dims}." - ) + msg = ( + f"Units of {units} specified in Patch.slope_filter, but units " + f"are not defined for both specified dimensions: {freq_dims}." + )
🧹 Nitpick comments (7)
dascore/utils/misc.py (1)
869-898: Top-level non-Mapping objects currently delegate to_robust_equality_check; recursion gap is real but not criticalThe review identifies a real asymmetry: while nested objects with
__dict__orequals()are handled recursively, top-level non-Mapping objects short-circuit to_robust_equality_check, which only compares via==operator. This means top-level objects without custom__eq__won't recurse into__dict__attributes.However, this gap does not affect current usage:
- Production call (spool.py:90): passes only Mapping objects (dicts)
- Tests for top-level objects (DataFrames, arrays, scalars): all use
_robust_equality_checkcorrectly via its==comparison logicThe suggested refactor is architecturally sound but addresses an untested edge case. If implementing it, correct the typo in the diff:
_robust_eility_check→_robust_equality_check.dascore/proc/filter.py (2)
603-606: Addstacklevelto DeprecationWarning.Surface deprecation at the caller site to aid migration.
Apply this diff:
- warnings.warn(msg, DeprecationWarning) + warnings.warn(msg, DeprecationWarning, stacklevel=2)
492-505: Update examples to avoid promoting deprecatednotcharg.Docs should show
invert=instead of deprecatednotch=.Apply this diff:
- >>> patch_filtered = patch.slope_filter( - ... filt=filt, - ... directional=False, - ... notch=False - ... ) + >>> patch_filtered = patch.slope_filter( + ... filt=filt, + ... directional=False, + ... invert=False, + ... ) @@ - >>> # Example 2: Notch filter - >>> patch_filtered = patch.slope_filter(filt=filt, notch=True) + >>> # Example 2: Inverted (notch) filter + >>> patch_filtered = patch.slope_filter(filt=filt, invert=True)Also applies to: 513-519
dascore/proc/mute.py (3)
90-109: Usestrict=Trueinzipto catch length mismatches early.Prevents silent truncation when dimensions and smooth values diverge.
Apply this diff:
- def _convert_to_samples(smooth, dims, patch): + def _convert_to_samples(smooth, dims, patch): """Convert the smooth parameter to number of samples.""" out = [] - for dim, val in zip(dims, smooth): + for dim, val in zip(dims, smooth, strict=True): coord = patch.get_coord(dim)
249-257: Addstrict=Truetozip(coords, value_list).Guards against silent shape drift between coords and provided values.
Apply this diff:
- for ind, (coord, row) in enumerate(zip(coords, value_list)): + for ind, (coord, row) in enumerate(zip(coords, value_list, strict=True)):
125-132: Remove or underscore unused parameterpatchin_MuteGeometry1D.from_params.Avoids ARG003 warnings and clarifies intent.
Apply this diff:
- def from_params(cls, vals, dims, axes, patch, relative): + def from_params(cls, vals, dims, axes, _patch, relative):tests/test_proc/test_mute.py (1)
139-144: Silence unused variable in test (v1).
v1is assigned but not used in this test. Rename to_(or_v1) to satisfy linters without altering behavior.Apply this diff:
- v1, v2 = _get_testable_coord_values(coord, relative=True) + _v1, v2 = _get_testable_coord_values(coord, relative=True)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
benchmarks/test_patch_benchmarks.py(1 hunks)dascore/core/patch.py(2 hunks)dascore/proc/__init__.py(1 hunks)dascore/proc/basic.py(1 hunks)dascore/proc/filter.py(6 hunks)dascore/proc/mute.py(1 hunks)dascore/utils/misc.py(1 hunks)dascore/utils/signal.py(1 hunks)tests/test_proc/test_basic.py(1 hunks)tests/test_proc/test_filter.py(1 hunks)tests/test_proc/test_mute.py(1 hunks)tests/test_proc/test_taper.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
tests/test_proc/test_basic.py (5)
dascore/examples.py (1)
random_patch(29-111)tests/conftest.py (2)
random_patch(282-284)patch(375-377)dascore/proc/basic.py (1)
full(795-819)dascore/core/patch.py (2)
coords(255-269)data(272-283)dascore/core/coords.py (1)
data(606-608)
dascore/core/patch.py (2)
dascore/proc/basic.py (1)
full(795-819)dascore/proc/mute.py (2)
line_mute(411-527)slope_mute(532-634)
dascore/proc/basic.py (1)
dascore/utils/patch.py (1)
patch_function(183-289)
tests/test_proc/test_taper.py (3)
tests/test_proc/test_mute.py (1)
patch_ones(62-64)dascore/proc/taper.py (1)
taper_range(200-289)dascore/proc/coords.py (1)
get_coord(96-148)
dascore/proc/mute.py (7)
dascore/proc/filter.py (1)
gaussian_filter(384-438)dascore/exceptions.py (1)
ParameterError(26-27)dascore/utils/docs.py (1)
compose_docstring(45-97)dascore/utils/misc.py (1)
get_2d_line_intersection(900-928)dascore/utils/models.py (1)
DascoreBaseModel(88-121)dascore/utils/patch.py (2)
get_dim_axis_value(562-646)patch_function(183-289)dascore/core/coords.py (1)
get_sample_count(757-793)
benchmarks/test_patch_benchmarks.py (2)
tests/conftest.py (1)
patch(375-377)dascore/proc/mute.py (2)
line_mute(411-527)slope_mute(532-634)
dascore/proc/__init__.py (1)
dascore/proc/mute.py (2)
line_mute(411-527)slope_mute(532-634)
tests/test_proc/test_mute.py (6)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/utils/time.py (1)
to_float(327-333)dascore/core/patch.py (6)
data(272-283)dims(212-226)ndim(229-231)coords(255-269)Patch(28-514)shape(286-299)dascore/core/coords.py (1)
get_next_index(795-873)dascore/examples.py (1)
random_patch(29-111)dascore/proc/mute.py (2)
line_mute(411-527)slope_mute(532-634)
tests/test_proc/test_filter.py (2)
dascore/proc/filter.py (1)
slope_filter(442-613)dascore/core/patch.py (1)
Patch(28-514)
🪛 Ruff (0.14.1)
tests/test_proc/test_basic.py
835-835: Pointless comparison. Did you mean to assign a value? Otherwise, prepend assert or remove it.
(B015)
dascore/proc/filter.py
605-605: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
dascore/proc/mute.py
93-93: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
125-125: Unused class method argument: patch
(ARG003)
249-249: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
tests/test_proc/test_mute.py
141-141: Unpacked variable v1 is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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 (windows-latest, 3.12)
- GitHub Check: test_code_min_deps (windows-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 (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (macos-latest, 3.11)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.13)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- GitHub Check: Run benchmarks
🔇 Additional comments (9)
dascore/utils/signal.py (1)
22-22: LGTM! Boxcar window function added correctly.The addition of the boxcar window alias follows the same pattern as other window functions in the dictionary and properly maps to
windows.boxcarfrom scipy.dascore/proc/__init__.py (1)
15-15: LGTM! Mute functions properly exposed.The import statement correctly exposes
line_muteandslope_mutein thedascore.procnamespace, following the same pattern as other processing functions.dascore/proc/basic.py (1)
794-819: LGTM! Clean and well-documented implementation.The
fullfunction is implemented correctly usingnp.fulland follows the established pattern of other functions in this module. The docstring provides clear examples of usage.benchmarks/test_patch_benchmarks.py (1)
148-162: LGTM! Benchmarks properly test new mute functionality.Both benchmark tests are well-structured and exercise the key features of the new mute functions:
test_line_mutecovers 2D muting with velocity lines and smoothingtest_slope_mutecovers slope-based muting between velocitiestests/test_proc/test_taper.py (2)
29-30: Good call skipping boxcar in general taper tests.Since the boxcar window doesn't actually apply tapering (it's a rectangular window with all values = 1), it's appropriate to skip it in the
time_tapered_patchfixture that tests general taper behavior.
280-311: LGTM! Comprehensive boxcar window tests.Both tests appropriately validate boxcar window behavior:
test_boxcar_windowconfirms the window produces uniform values (1.0 without invert, 0.0 with invert)test_two_value_invert_mutes_to_last_samplevalidates 4-value taper specification with specific index assertionsdascore/core/patch.py (2)
367-367: LGTM! Patch method alias added correctly.The
fullmethod is properly exposed as a Patch attribute, following the same pattern as other processing functions.
442-443: LGTM! Mute methods properly exposed.Both
line_muteandslope_muteare correctly aliased as Patch methods, positioned appropriately near thetaperandtaper_rangemethods they complement.tests/test_proc/test_filter.py (1)
406-416: LGTM! Tests properly reflect the API deprecation.The changes correctly test both the new
invertparameter and ensure backward compatibility:
test_invert_filtervalidates the newinvert=Trueparameter workstest_notch_deprecatedconfirms that using the deprecatednotchparameter triggers aDeprecationWarningThis provides good coverage for the API transition.
CodSpeed Performance ReportMerging #579 will not alter performanceComparing Summary
Benchmarks breakdown
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dascore/proc/mute.py (1)
603-606: Clarify error message for slope validation.The error message states "slopes must be positive" but the code explicitly handles zero slopes (line 618). Update the message to be more accurate.
Apply this diff:
- if np.any(slopes_array < 0): - msg = "slopes must be positive." + if np.any(slopes_array < 0): + msg = "slopes must be non-negative (>= 0)." raise ParameterError(msg)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dascore/proc/filter.py(8 hunks)dascore/proc/mute.py(1 hunks)dascore/utils/misc.py(1 hunks)tests/test_proc/test_basic.py(1 hunks)tests/test_proc/test_mute.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- dascore/utils/misc.py
- tests/test_proc/test_basic.py
🧰 Additional context used
🧬 Code graph analysis (2)
dascore/proc/mute.py (6)
dascore/proc/filter.py (1)
gaussian_filter(384-438)dascore/utils/docs.py (1)
compose_docstring(45-97)dascore/utils/misc.py (1)
get_2d_line_intersection(900-928)dascore/utils/models.py (1)
DascoreBaseModel(88-121)dascore/utils/patch.py (2)
get_dim_axis_value(562-646)patch_function(183-289)dascore/core/coords.py (1)
get_sample_count(757-793)
tests/test_proc/test_mute.py (4)
dascore/exceptions.py (1)
ParameterError(26-27)dascore/utils/time.py (1)
to_float(327-333)dascore/core/coords.py (1)
get_next_index(795-873)dascore/proc/mute.py (2)
line_mute(411-527)slope_mute(532-634)
🪛 Ruff (0.14.1)
dascore/proc/mute.py
125-125: Unused class method argument: patch
(ARG003)
⏰ 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 (macos-latest, 3.11)
- GitHub Check: test_code (windows-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.12)
- GitHub Check: test_code (windows-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.11)
- GitHub Check: test_code (macos-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.13)
- GitHub Check: test_code (ubuntu-latest, 3.10)
- GitHub Check: test_code (ubuntu-latest, 3.12)
- 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 (ubuntu-latest, 3.13)
- GitHub Check: test_code_min_deps (windows-latest, 3.13)
- GitHub Check: test_code_min_deps (macos-latest, 3.13)
- GitHub Check: Run benchmarks
🔇 Additional comments (16)
dascore/proc/filter.py (4)
11-11: LGTM!The
warningsimport is correctly added to support the deprecation warning for thenotchparameter.
476-480: LGTM!The docstring updates clearly mark
notchas deprecated and provide comprehensive documentation for the newinvertparameter.
504-504: LGTM!The examples are correctly updated to demonstrate the new
invertparameter while maintaining clarity. The comment on line 513 helpfully retains the "notch" terminology for users familiar with that concept.Also applies to: 513-514
533-533: LGTM!The internal
_get_taper_maskfunction signature and implementation are correctly updated fromnotchtoinvert, maintaining the same semantics and behavior.Also applies to: 546-546, 608-608
tests/test_proc/test_mute.py (6)
12-59: Test helper functions are well-designed.The helper functions properly abstract common test patterns: computing representative coordinate values, validating data ranges, and checking specific point values. The logic is clear and reusable across multiple test cases.
61-92: Fixtures provide comprehensive test coverage.The fixtures create patches of varying dimensionality (2D, 3D, 4D) with ones, enabling thorough testing of mute behavior across different data shapes.
94-386: Comprehensive test coverage for line mute functionality.The tests effectively validate:
- Error handling for invalid parameters
- 1D muting with various smoothing configurations
- 2D line muting with parallel and non-parallel lines
- Implicit value handling (None, ...)
- Both relative and absolute coordinate modes
388-472: Smoothing tests validate gradual transitions.The tests properly verify that smoothing creates intermediate values and works correctly with dict parameters, invert mode, and various edge cases.
474-597: Higher-dimensional mute tests ensure proper broadcasting.The 3D and 4D tests verify that muting operates correctly on subspaces while preserving the full dimensionality of the patch, confirming proper broadcasting behavior.
599-804: Slope mute tests cover critical edge cases.The tests properly validate:
- Basic slope-based muting with velocity interpretations
- Edge cases including zero and infinite slopes
- Negative slope rejection
- Equivalence with manual line_mute calls
- Custom dimension ordering (velocity vs slowness)
dascore/proc/mute.py (6)
1-41: Imports and documentation constants are appropriate.The imports are well-organized, and the
_smooth_paramdocstring constant enables consistent documentation across functions.
43-115: Well-structured geometry abstraction.The abstract base class properly defines the interface for different mute geometries, with clear separation between 1D and 2D cases. The smoothing parameter handling is flexible and validates input ranges appropriately.
117-141: 1D geometry implementation is correct.The implementation properly handles single-dimension muting. The static analysis hint about the unused
patchparameter is a false positive—the parameter is required to match the abstract method signature and maintain interface consistency with_MuteGeometry2D.
334-377: Geometrically sound region detection.The cross product and dot product logic correctly determines which points lie within the mute region. The distinction between parallel and non-parallel cases is properly handled.
380-407: Clean geometry factory function.The function properly validates inputs and dispatches to the appropriate geometry class based on dimensionality.
409-634: Excellent documentation with practical examples.Both
line_muteandslope_muteprovide comprehensive docstrings with multiple usage examples, clear parameter descriptions, and helpful notes about behavior and limitations.
|
Hey @ahmadtourei, I hate to bother you again, but if you aren't too busy, could you take a look at this one? I think the implementation is solid, I just want your take on the API for line_mute and slope_mute as I am not sure I got it 100% right. |
|
Ok, I am going to merge this for now. There may be some tweaking to do later but I think it is pretty solid as is. |
Hey @d-chambers, sorry I'm not sure how I completely missed the email notification from this PR. I will take a look next few days and will get back to you if I have any comments. Also, I'll have more time to work on DASCore stuff over the next couple of months, so just let me know! |
No worries, sounds good. Even if we find some issues we can fix before next release. I am particularly interested in what you think about the API. |
| >>> # Line from (t=0, d=0) to (t=0.3, d=300) defines velocity=1000 m/s | ||
| >>> muted = patch.line_mute( | ||
| ... time=(0, [0, 0.3]), | ||
| ... distance=(None, [0, 300]), |
There was a problem hiding this comment.
A bit confusing why we have None for distance here, and not for time kwargs. Wouldn't time=(0, 0.3), distance=(0, 300) work?
There was a problem hiding this comment.
Ah I guess this is because we want to apply the same mute to the channels with negative distance values (if they exist) as well?
There was a problem hiding this comment.
A bit confusing why we have None for distance here, and not for time kwargs. Wouldn't time=(0, 0.3), distance=(0, 300) work?
So this is to say all distances and time=0. Essentially a horizontal line if time is on the x axis.
| >>> # Mute wedge between two velocity lines | ||
| >>> muted = patch.line_mute( | ||
| ... time=([0, 0.375], [0, 0.25]), | ||
| ... distance=([0, 300], [0, 300]), |
There was a problem hiding this comment.
Could be distance=(None, [0, 300])?
There was a problem hiding this comment.
No, we would need a single time value to pair with the None in distance (eg so it is another horizontal line). If time = None and distance = 1 it would be a vertical line. Does that make sense? Maybe I can make the docs more clear about this...
There was a problem hiding this comment.
Cool, yes that would be great to add a quick note in the docs about this.
Description
This PR adds two new patch methods: line_mute and slope_mute. Line mute is generic; it allows the specification of two lines and applies a mute to the area between the mutes. The slope_mute just calculates the lines from the specified slopes and calls the appropriate function.
Checklist
I have (if applicable):
Summary by CodeRabbit
New Features
Changes
Tests
Benchmarks