add wav write support for patches with non-distance dimensions #488
Conversation
WalkthroughThe changes update the WAV file writing logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant W as WavIO
participant FS as File System
C->>W: write(patch, resource, ...)
W->>W: Determine non-time dimension dynamically
W->>W: Process patch data & compute sample rate using time step
W->>W: Invoke _get_wav_data(patch, resample)
W->>FS: Write WAV data to directory based on patch dimensions
FS-->>W: Confirmation of write
W-->>C: Return success
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (15)
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: 0
🔭 Outside diff range comments (1)
dascore/io/wav/core.py (1)
20-55: Update docstring to reflect non-distance dimension support.The docstring still references "distance" dimension in multiple places. Update it to reflect that any non-time dimension is supported.
Example enhancement:
- If a path that ends with .wav, write all the distance channels + If a path that ends with .wav, write all channels from the non-time dimension to a single file. If not, assume the path is a directory and write - each distance channel to its own wav file. + each channel to its own wav file. ... - the output the patch has more than one len along the distance - dimension, a multi-channel wavefile is created. There may be some + the output patch has more than one value along the non-time + dimension, a multi-channel wavefile is created. There may be some
🧹 Nitpick comments (1)
tests/test_io/test_wav/test_wav.py (1)
53-60: Enhance test coverage for non-distance dimension WAV writing.While the test verifies basic functionality, consider adding assertions to:
- Verify the number of WAV files matches the number of microphone coordinates
- Check sample rate and data integrity of written files
- Validate the naming pattern of generated files
Example enhancement:
def test_write_non_distance_dims( self, audio_patch_non_distance_dim, tmp_path_factory ): """Ensure any non-time dimension still works.""" path = tmp_path_factory.mktemp("wav_resample") patch = audio_patch_non_distance_dim patch.io.write(path, "wav") assert path.exists() + # Verify number of WAV files + wavs = list(path.rglob("*.wav")) + assert len(wavs) == len(patch.coords.get_array("microphone")) + # Verify file naming + for mic_val in patch.coords.get_array("microphone"): + assert path / f"microphone_{mic_val}.wav" in wavs + # Verify content of first file + sr, data = read_wav(str(wavs[0])) + assert sr == int(ONE_SECOND / patch.get_coord("time").step)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/io/wav/core.py(1 hunks)tests/test_io/test_wav/test_wav.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_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 (ubuntu-latest, 3.11)
- GitHub Check: test_code_min_deps (macos-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)
🔇 Additional comments (3)
tests/test_io/test_wav/test_wav.py (1)
28-32: LGTM! Well-structured fixture for testing non-distance dimensions.The fixture correctly creates a test patch by renaming the "distance" coordinate to "microphone", enabling testing of WAV writing functionality with non-distance dimensions.
dascore/io/wav/core.py (2)
82-95: LGTM! Robust handling of patch data preparation.The changes correctly:
- Verify time dimension presence using
check_patch_coords- Ensure proper data shape with time as first dimension
- Calculate sample rate from time coordinate step
63-77: Consider handling multiple non-time dimensions.The current implementation assumes exactly one non-time dimension. Consider handling cases where patches might have multiple non-time dimensions.
Example enhancement:
- non_time_name = next( - iter( - set(patch.dims) - - { - "time", - } - ) - ) + non_time_dims = set(patch.dims) - {"time"} + if len(non_time_dims) > 1: + raise ValueError( + f"Writing WAV files with multiple non-time dimensions {non_time_dims} " + "is not supported" + ) + non_time_name = next(iter(non_time_dims))
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #488 +/- ##
=======================================
Coverage 99.85% 99.85%
=======================================
Files 118 118
Lines 9693 9696 +3
=======================================
+ Hits 9679 9682 +3
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. |
Docstrings generation was requested by @d-chambers. * #488 (comment) The following files were modified: * `dascore/io/wav/core.py` * `tests/test_io/test_wav/test_wav.py`
|
Note Generated docstrings for this pull request at #489 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_io/test_wav/test_wav.py (1)
55-71: Add verification of WAV data content.The test thoroughly checks file existence, count, and naming, but it's missing verification of the actual WAV data content. Consider adding assertions to verify that the data in the WAV files matches the expected values from the patch.
def test_write_non_distance_dims( self, audio_patch_non_distance_dim, tmp_path_factory ): """Ensure any non-time dimension still works.""" path = tmp_path_factory.mktemp("wav_resample") patch = audio_patch_non_distance_dim patch.io.write(path, "wav") assert path.exists() # Verify number of WAV files wavs = list(path.rglob("*.wav")) assert len(wavs) == len(patch.coords.get_array("microphone")) # Verify file naming for mic_val in patch.coords.get_array("microphone"): assert path / f"microphone_{mic_val}.wav" in wavs # Verify content of first file sr, data = read_wav(str(wavs[0])) assert sr == int(ONE_SECOND / patch.get_coord("time").step) + # Verify data content + for wav_path, mic_val in zip(sorted(wavs), patch.coords.get_array("microphone")): + sr, data = read_wav(str(wav_path)) + expected_data = patch.sel(microphone=mic_val).data + # Compare data after accounting for normalization + expected_data = (expected_data - expected_data.mean()) / abs(expected_data).max() + np.testing.assert_allclose(data, expected_data, rtol=1e-5)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dascore/io/wav/core.py(3 hunks)tests/test_io/test_wav/test_wav.py(3 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_min_deps (windows-latest, 3.13)
- 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.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 (3)
tests/test_io/test_wav/test_wav.py (1)
30-34: LGTM! Well-structured fixture.The fixture is well-documented and follows pytest fixture patterns. The scope matches other fixtures, and the implementation is clear.
dascore/io/wav/core.py (2)
29-31: LGTM! Clear and accurate docstring updates.The docstring updates accurately reflect the changes in functionality, maintaining clarity and consistency.
Also applies to: 48-49
77-80: LGTM! Robust handling of non-time dimensions.The changes correctly handle non-time dimensions while maintaining proper validation and sample rate calculation. The transposition ensures consistent data layout.
Also applies to: 82-82, 88-88
| else: # write data to directory, one file for each non-time | ||
| resource.mkdir(exist_ok=True, parents=True) | ||
| distances = patch.coords.get_array("distance") | ||
| for ind, dist in enumerate(distances): | ||
| non_time_set = set(patch.dims) - {"time"} | ||
| non_time_name = next(iter(non_time_set)) | ||
| non_time = patch.coords.get_array(non_time_name) | ||
| for ind, val in enumerate(non_time): | ||
| sub_data = np.take(data, ind, axis=1) | ||
| sub_path = resource / f"{dist}.wav" | ||
| sub_path = resource / f"{non_time_name}_{val}.wav" | ||
| write(filename=str(sub_path), rate=int(sr), data=sub_data) |
There was a problem hiding this comment.
Add validation for multiple non-time dimensions.
The code assumes only one non-time dimension exists but doesn't validate this assumption. This could lead to cryptic errors if a patch has multiple non-time dimensions.
else: # write data to directory, one file for each non-time
resource.mkdir(exist_ok=True, parents=True)
non_time_set = set(patch.dims) - {"time"}
+ if len(non_time_set) != 1:
+ raise ValueError(
+ f"Expected exactly one non-time dimension, got {len(non_time_set)}: {non_time_set}"
+ )
non_time_name = next(iter(non_time_set))
non_time = patch.coords.get_array(non_time_name)
for ind, val in enumerate(non_time):
sub_data = np.take(data, ind, axis=1)
sub_path = resource / f"{non_time_name}_{val}.wav"
write(filename=str(sub_path), rate=int(sr), data=sub_data)Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This PR simply allows patches that have a time dimension and a non-distance dimension to be written as a single wav file or a directory of wav files.
Checklist
I have (if applicable):
Summary by CodeRabbit