Conversation
add rounding
Fix wischart
Refactoring
|
Output of |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR refactors dataset generation and testing infrastructure, adds optional impulse-response (IR) support and a new SRIRACHA dataset path, and modernizes TFRecord/HDF5 serialization and project tooling (uv, org CI, docs theme).
Changes:
- Centralizes pytest fixtures into
tests/conftest.py, migrates dataset validation from.npyfiles to snapshot-style checks, and adds IR optional-dependency tests. - Refactors feature construction to use
BaseFeatureCatalog+BaseFeatureCollectionBuilder, adds TFRecord encoding inference + dynamic-shape metadata, and introducesremote_argsfor Ray actor resource configuration. - Adds IR helper module (
acoupipe.datasets.ir), new SRIRACHA dataset wiring/docs, and updates packaging/docs/CI to a uv + org-workflow setup.
Reviewed changes
Copilot reviewed 47 out of 335 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_spectra.py | Removes local fixtures/imports in favor of shared conftest.py fixtures. |
| tests/test_sampler.py | Switches to shared fixtures; trims imports now provided via conftest.py. |
| tests/test_loader.py | Moves HDF5 temp-file fixture to conftest.py. |
| tests/test_ir.py | Adds tests for IR backend fallback and optional dependency error reporting. |
| tests/test_datasets.py | Migrates dataset value checks to snapshot-based assertions; adjusts multiprocessing skips. |
| tests/test_dataset_tf.py | Removes duplicated temp-dir/dataset fixtures to rely on conftest.py. |
| tests/create_validation_data.py | Deletes legacy script for creating .npy validation data. |
| tests/conftest.py | Introduces shared fixtures for datasets, samplers, pipelines, and HDF5 temp data. |
| src/acoupipe/writer.py | Refactors TFRecord encoders, adds dtype/shape inference and optional shape sidecar features. |
| src/acoupipe/sampler.py | Changes LocationSampler.sample_loc and how locations are applied to targets. |
| src/acoupipe/pipeline.py | Adds Ray remote_args plumbing and debug logging in SamplerActor. |
| src/acoupipe/loader.py | Wraps metadata loading with a catch-all exception handler and prints errors. |
| src/acoupipe/datasets/utils.py | Adds new calc_transfer and removes blockwise_transfer. |
| src/acoupipe/datasets/synthetic.py | Major feature-builder refactor; adds IR-backed ISM dataset/config; passes remote_args. |
| src/acoupipe/datasets/spectra_analytic.py | Adds effective DoF computation (df_eq) and changes window handling. |
| src/acoupipe/datasets/micgeom.py | Replaces hardcoded arrays with loading mic geometry from Acoular XML. |
| src/acoupipe/datasets/ir.py | Adds optional IR backends (gpuRIR/pyroomacoustics) with guided ImportErrors. |
| src/acoupipe/datasets/features.py | Moves to data-dependent feature execution; adds TF encoder inference integration. |
| src/acoupipe/datasets/experimental.py | Extends scenarios (SRIRACHA), refactors IR handling, adds remote_args passthrough. |
| src/acoupipe/datasets/base.py | Refactors default-feature building and changes public dataset method signatures; adds remote_args. |
| pyproject.toml | Updates Python/acoular constraints, replaces extras with dependency groups, adds pytest config and uv setup. |
| examples/point_source_example.py | Updates grid extent usage for plotting. |
| docs/source/index.rst | Updates datasets overview and citations; adjusts figure sizing and content. |
| docs/source/contents/script/quickstart.py | Updates plotting extent usage. |
| docs/source/contents/script/features.py | Updates plotting extent usage. |
| docs/source/contents/script/experimental.py | Updates example frequency and formatting; switches to grid extent property. |
| docs/source/contents/script/all_datasets.py | Adds a new script generating comparative plots across datasets/scenarios. |
| docs/source/contents/jupyter/modify.ipynb | Clears execution counts and updates extent usage. |
| docs/source/contents/jupyter/load_tfrecord.ipynb | Clears execution counts and updates extent usage. |
| docs/source/contents/jupyter/generate.ipynb | Updates extent usage. |
| docs/source/contents/install.rst | Rewrites install docs around uv/pip and dependency groups; updates Docker snippet formatting. |
| docs/source/contents/datasets/synthetic.rst | Adds dedicated DatasetSynthetic documentation page. |
| docs/source/contents/datasets/store.rst | Adds documentation page for HDF5/TFRecord storage. |
| docs/source/contents/datasets/sriracha.rst | Adds dedicated DatasetSRIRACHA documentation page. |
| docs/source/contents/datasets/miracle.rst | Adds dedicated DatasetMIRACLE documentation page. |
| docs/source/contents/datasets.rst | Replaces autoapi dataset pages with curated dataset pages + store page. |
| docs/source/conf.py | Switches theme to pydata-sphinx-theme, adds sphinx-design, configures AutoAPI skips. |
| docs/source/bib/refs.bib | Adds SRIRACHA and MIRACLE citations; expands AcouPipe reference fields. |
| docs/source/_static/css/custom_pydata_sphinx_theme.css | Adds custom layout styling for pydata-sphinx-theme. |
| README.rst | Major rewrite: installation, datasets overview (adds SRIRACHA), quickstart, citations. |
| .python-version | Pins dev Python to 3.13. |
| .github/workflows/tests.yml | Removes local test workflow (replaced by organizational CI). |
| .github/workflows/orga-ci.yml | Adds organizational CI workflow calls (tests, coverage, docs, lint, links). |
| .github/workflows/docs.yml | Migrates docs build to uv and Python 3.13; removes pull_request trigger. |
| .github/workflows/checks.yml | Removes local lint/link/cff workflows (replaced by organizational CI). |
| .github/actions/setup-hatch/action.yml | Removes hatch setup action (uv-based workflows replace it). |
Comments suppressed due to low confidence (1)
src/acoupipe/sampler.py:1
- This change is very likely to break
LocationSampler:target.locis commonly a tuple (e.g.(x, y, z)), sotarget.loc.copy()will raiseAttributeError. Even iftarget.locis an array,loc[loc_axs, 0]assumes an extra dimension (shape(3, 1)), which won’t hold for a 1D(3,)location vector. Finally, assigningtarget.loc = new_locchanges the type from tuple to ndarray, which can break downstream Acoular expectations. Suggested fix: keep the previous conversion pattern (np.array(target.loc)) and update elements with 1D indexing (e.g.loc[loc_axs] = ...), and assign back using the expected type (likelytuple(loc)).
"""Random processes to sample values according to a specified random distribution (random variable).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from numpy import concatenate, imag, newaxis, real, searchsorted | ||
|
|
||
|
|
||
| def calc_transfer(ir, fs, blocksize, fftfreq, time_axis=-1): |
There was a problem hiding this comment.
calc_transfer() currently ignores time_axis (padding always uses -1 and the FFT always uses axis=-1), assumes fftfreq is always a numeric array (will crash if None despite the docstring), and always returns H_sel.T which will reorder axes incorrectly for inputs with ndim>2. Additionally, the docstring claims integer fftfreq should be treated as bin indices, but the implementation always treats fftfreq as Hz. Suggested fix: honor time_axis via np.moveaxis, pad along the actual time_axis, handle fftfreq is None by returning all bins, and implement the integer-bin-index path separately (no Hz-to-bin conversion). Also remove the unused np.fft.rfftfreq(...) line or store it for validation.
| L = ir.shape[time_axis] | ||
|
|
||
| # Select FFT length: power-of-two >= max(L, blocksize) | ||
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | ||
|
|
||
| # Zero-pad IRs to nfft (never truncate) | ||
| if nfft > L: | ||
| pad_width = [(0, 0)] * ir.ndim | ||
| pad_width[-1] = (0, nfft - L) | ||
| ir = np.pad(ir, pad_width, mode='constant') | ||
|
|
||
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | ||
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | ||
| np.fft.rfftfreq(nfft, d=1.0 / fs) | ||
|
|
||
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | ||
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | ||
| raise ValueError(msg) | ||
|
|
||
| # Map to nearest bin; require exact (within tolerance) grid match | ||
| idx_float = fftfreq * nfft / fs | ||
| idx = np.rint(idx_float).astype(int) | ||
| tol = 1e-9 | ||
| if np.max(np.abs(idx_float - idx)) > tol: | ||
| msg = ( | ||
| 'Some requested frequencies are not on the FFT grid. ' | ||
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| H_sel = np.take(H, idx, axis=-1) | ||
| return H_sel.T | ||
|
|
||
|
|
There was a problem hiding this comment.
calc_transfer() currently ignores time_axis (padding always uses -1 and the FFT always uses axis=-1), assumes fftfreq is always a numeric array (will crash if None despite the docstring), and always returns H_sel.T which will reorder axes incorrectly for inputs with ndim>2. Additionally, the docstring claims integer fftfreq should be treated as bin indices, but the implementation always treats fftfreq as Hz. Suggested fix: honor time_axis via np.moveaxis, pad along the actual time_axis, handle fftfreq is None by returning all bins, and implement the integer-bin-index path separately (no Hz-to-bin conversion). Also remove the unused np.fft.rfftfreq(...) line or store it for validation.
| L = ir.shape[time_axis] | |
| # Select FFT length: power-of-two >= max(L, blocksize) | |
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir = np.pad(ir, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| np.fft.rfftfreq(nfft, d=1.0 / fs) | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return H_sel.T | |
| ir = np.asarray(ir) | |
| ir_fft = np.moveaxis(ir, time_axis, -1) | |
| L = ir_fft.shape[-1] | |
| # Select FFT length: power-of-two >= max(L, blocksize) | |
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir_fft.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir_fft = np.pad(ir_fft, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir_fft, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| freq_grid = np.fft.rfftfreq(nfft, d=1.0 / fs) | |
| if fftfreq is None: | |
| H_sel = H | |
| else: | |
| fftfreq = np.asarray(fftfreq) | |
| if np.issubdtype(fftfreq.dtype, np.integer): | |
| if np.any(fftfreq < 0) or np.any(fftfreq > blocksize // 2): | |
| msg = 'Requested bin indices must satisfy 0 <= k <= blocksize//2.' | |
| raise ValueError(msg) | |
| ratio = nfft // blocksize | |
| idx = fftfreq.astype(int) * ratio | |
| else: | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(freq_grid[idx] - fftfreq)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return np.moveaxis(H_sel, -1, time_axis) |
| L = ir.shape[time_axis] | ||
|
|
||
| # Select FFT length: power-of-two >= max(L, blocksize) | ||
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | ||
|
|
||
| # Zero-pad IRs to nfft (never truncate) | ||
| if nfft > L: | ||
| pad_width = [(0, 0)] * ir.ndim | ||
| pad_width[-1] = (0, nfft - L) | ||
| ir = np.pad(ir, pad_width, mode='constant') | ||
|
|
||
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | ||
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | ||
| np.fft.rfftfreq(nfft, d=1.0 / fs) | ||
|
|
||
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | ||
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | ||
| raise ValueError(msg) | ||
|
|
||
| # Map to nearest bin; require exact (within tolerance) grid match | ||
| idx_float = fftfreq * nfft / fs | ||
| idx = np.rint(idx_float).astype(int) | ||
| tol = 1e-9 | ||
| if np.max(np.abs(idx_float - idx)) > tol: | ||
| msg = ( | ||
| 'Some requested frequencies are not on the FFT grid. ' | ||
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| H_sel = np.take(H, idx, axis=-1) | ||
| return H_sel.T | ||
|
|
||
|
|
There was a problem hiding this comment.
calc_transfer() currently ignores time_axis (padding always uses -1 and the FFT always uses axis=-1), assumes fftfreq is always a numeric array (will crash if None despite the docstring), and always returns H_sel.T which will reorder axes incorrectly for inputs with ndim>2. Additionally, the docstring claims integer fftfreq should be treated as bin indices, but the implementation always treats fftfreq as Hz. Suggested fix: honor time_axis via np.moveaxis, pad along the actual time_axis, handle fftfreq is None by returning all bins, and implement the integer-bin-index path separately (no Hz-to-bin conversion). Also remove the unused np.fft.rfftfreq(...) line or store it for validation.
| L = ir.shape[time_axis] | |
| # Select FFT length: power-of-two >= max(L, blocksize) | |
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir = np.pad(ir, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| np.fft.rfftfreq(nfft, d=1.0 / fs) | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return H_sel.T | |
| ir = np.asarray(ir) | |
| ir_work = np.moveaxis(ir, time_axis, -1) | |
| L = ir_work.shape[-1] | |
| # Select FFT length: power-of-two >= max(L, blocksize) | |
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir_work.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir_work = np.pad(ir_work, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir_work, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| if fftfreq is None: | |
| H_sel = H | |
| else: | |
| fftfreq = np.asarray(fftfreq) | |
| if np.issubdtype(fftfreq.dtype, np.integer): | |
| if np.any(fftfreq < 0) or np.any(fftfreq > blocksize // 2): | |
| msg = 'Requested bin indices must satisfy 0 <= k <= blocksize//2.' | |
| raise ValueError(msg) | |
| ratio = nfft // blocksize | |
| idx = fftfreq.astype(int) * ratio | |
| else: | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return np.moveaxis(H_sel, -1, time_axis) |
| L = ir.shape[time_axis] | ||
|
|
||
| # Select FFT length: power-of-two >= max(L, blocksize) | ||
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | ||
|
|
||
| # Zero-pad IRs to nfft (never truncate) | ||
| if nfft > L: | ||
| pad_width = [(0, 0)] * ir.ndim | ||
| pad_width[-1] = (0, nfft - L) | ||
| ir = np.pad(ir, pad_width, mode='constant') | ||
|
|
||
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | ||
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | ||
| np.fft.rfftfreq(nfft, d=1.0 / fs) | ||
|
|
||
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | ||
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | ||
| raise ValueError(msg) | ||
|
|
||
| # Map to nearest bin; require exact (within tolerance) grid match | ||
| idx_float = fftfreq * nfft / fs | ||
| idx = np.rint(idx_float).astype(int) | ||
| tol = 1e-9 | ||
| if np.max(np.abs(idx_float - idx)) > tol: | ||
| msg = ( | ||
| 'Some requested frequencies are not on the FFT grid. ' | ||
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| H_sel = np.take(H, idx, axis=-1) | ||
| return H_sel.T | ||
|
|
||
|
|
There was a problem hiding this comment.
calc_transfer() currently ignores time_axis (padding always uses -1 and the FFT always uses axis=-1), assumes fftfreq is always a numeric array (will crash if None despite the docstring), and always returns H_sel.T which will reorder axes incorrectly for inputs with ndim>2. Additionally, the docstring claims integer fftfreq should be treated as bin indices, but the implementation always treats fftfreq as Hz. Suggested fix: honor time_axis via np.moveaxis, pad along the actual time_axis, handle fftfreq is None by returning all bins, and implement the integer-bin-index path separately (no Hz-to-bin conversion). Also remove the unused np.fft.rfftfreq(...) line or store it for validation.
| L = ir.shape[time_axis] | |
| # Select FFT length: power-of-two >= max(L, blocksize) | |
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir = np.pad(ir, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| np.fft.rfftfreq(nfft, d=1.0 / fs) | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return H_sel.T | |
| ir = np.asarray(ir) | |
| time_axis = np.core.numeric.normalize_axis_index(time_axis, ir.ndim) | |
| L = ir.shape[time_axis] | |
| # Select FFT length: power-of-two >= max(L, blocksize) | |
| nfft = 1 << (max(L, blocksize) - 1).bit_length() | |
| # Work on a view with time axis last so padding/FFT/selection are applied consistently | |
| ir_moved = np.moveaxis(ir, time_axis, -1) | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir_moved.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir_moved = np.pad(ir_moved, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir_moved, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| if fftfreq is None: | |
| H_sel = H | |
| else: | |
| fftfreq = np.asarray(fftfreq) | |
| if np.issubdtype(fftfreq.dtype, np.integer): | |
| if np.any(fftfreq < 0) or np.any(fftfreq > blocksize // 2): | |
| msg = 'Requested bin indices must satisfy 0 <= k <= blocksize//2.' | |
| raise ValueError(msg) | |
| scale = nfft // blocksize | |
| idx = fftfreq.astype(int) * scale | |
| else: | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return np.moveaxis(H_sel, -1, time_axis) |
| # Zero-pad IRs to nfft (never truncate) | ||
| if nfft > L: | ||
| pad_width = [(0, 0)] * ir.ndim | ||
| pad_width[-1] = (0, nfft - L) | ||
| ir = np.pad(ir, pad_width, mode='constant') | ||
|
|
||
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | ||
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | ||
| np.fft.rfftfreq(nfft, d=1.0 / fs) | ||
|
|
||
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | ||
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | ||
| raise ValueError(msg) | ||
|
|
||
| # Map to nearest bin; require exact (within tolerance) grid match | ||
| idx_float = fftfreq * nfft / fs | ||
| idx = np.rint(idx_float).astype(int) | ||
| tol = 1e-9 | ||
| if np.max(np.abs(idx_float - idx)) > tol: | ||
| msg = ( | ||
| 'Some requested frequencies are not on the FFT grid. ' | ||
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | ||
| ) | ||
| raise ValueError(msg) | ||
|
|
||
| H_sel = np.take(H, idx, axis=-1) | ||
| return H_sel.T |
There was a problem hiding this comment.
calc_transfer() currently ignores time_axis (padding always uses -1 and the FFT always uses axis=-1), assumes fftfreq is always a numeric array (will crash if None despite the docstring), and always returns H_sel.T which will reorder axes incorrectly for inputs with ndim>2. Additionally, the docstring claims integer fftfreq should be treated as bin indices, but the implementation always treats fftfreq as Hz. Suggested fix: honor time_axis via np.moveaxis, pad along the actual time_axis, handle fftfreq is None by returning all bins, and implement the integer-bin-index path separately (no Hz-to-bin conversion). Also remove the unused np.fft.rfftfreq(...) line or store it for validation.
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir = np.pad(ir, pad_width, mode='constant') | |
| # One-sided FFT with window matching the (zero-padded) length (i.e., use full nfft) | |
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| np.fft.rfftfreq(nfft, d=1.0 / fs) | |
| if np.any(fftfreq < 0.0) or np.any(fftfreq > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match | |
| idx_float = fftfreq * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return H_sel.T | |
| # Move the requested time axis to the end so padding/FFT operate on the | |
| # actual time dimension regardless of the original layout. | |
| ir = np.moveaxis(ir, time_axis, -1) | |
| # Zero-pad IRs to nfft (never truncate) | |
| if nfft > L: | |
| pad_width = [(0, 0)] * ir.ndim | |
| pad_width[-1] = (0, nfft - L) | |
| ir = np.pad(ir, pad_width, mode='constant') | |
| # One-sided FFT along the time axis (now the last axis). | |
| H = np.fft.rfft(ir, n=nfft, axis=-1) # shape: (..., nfft//2+1) | |
| if fftfreq is None: | |
| idx = np.arange(H.shape[-1]) | |
| else: | |
| fftfreq_arr = np.asarray(fftfreq) | |
| # Integer inputs are interpreted as direct FFT bin indices. | |
| if np.issubdtype(fftfreq_arr.dtype, np.integer): | |
| idx = fftfreq_arr.astype(int, copy=False) | |
| if np.any(idx < 0) or np.any(idx > nfft // 2): | |
| msg = 'Requested FFT bin indices must satisfy 0 <= k <= nfft//2.' | |
| raise ValueError(msg) | |
| else: | |
| # Non-integer inputs are interpreted as frequencies in Hz. | |
| if np.any(fftfreq_arr < 0.0) or np.any(fftfreq_arr > fs / 2.0): | |
| msg = 'Requested frequencies must satisfy 0 <= f <= fs/2.' | |
| raise ValueError(msg) | |
| # Map to nearest bin; require exact (within tolerance) grid match. | |
| idx_float = fftfreq_arr * nfft / fs | |
| idx = np.rint(idx_float).astype(int) | |
| tol = 1e-9 | |
| if np.max(np.abs(idx_float - idx)) > tol: | |
| msg = ( | |
| 'Some requested frequencies are not on the FFT grid. ' | |
| 'Provide integer bin indices (recommended), or choose f that matches k*fs/nfft.' | |
| ) | |
| raise ValueError(msg) | |
| H_sel = np.take(H, idx, axis=-1) | |
| return np.moveaxis(H_sel, -1, 0) |
| builder.add_custom(self.config.get_prepare_func()) # add prepare function | ||
| return builder.build() # finally build the feature collection | ||
|
|
||
| def generate(self, features, size, split='training', f=None, num=0, start_idx=0, progress_bar=True): |
There was a problem hiding this comment.
The public API method signatures were reordered (e.g., generate(features, size, split=...) instead of generate(features, split, size, ...)), which is a breaking change for callers using positional arguments. Suggested fix (mandatory if backward compatibility matters): keep the old positional order and introduce keyword-only parameters for the new preferred order, or accept both orders via deprecation handling (e.g., detect split passed as int/size passed as str).
| def generate(self, features, size, split='training', f=None, num=0, start_idx=0, progress_bar=True): | |
| def generate(self, features, split='training', size=None, f=None, num=0, start_idx=0, progress_bar=True): |
| yield from pipeline.get_data(progress_bar=progress_bar, start_idx=start_idx) | ||
|
|
||
| def save_h5(self, features, split, size, name, f=None, num=0, start_idx=0, progress_bar=True): | ||
| def save_h5(self, features, size, name, split='training', f=None, num=0, start_idx=0, progress_bar=True): |
There was a problem hiding this comment.
The public API method signatures were reordered (e.g., generate(features, size, split=...) instead of generate(features, split, size, ...)), which is a breaking change for callers using positional arguments. Suggested fix (mandatory if backward compatibility matters): keep the old positional order and introduce keyword-only parameters for the new preferred order, or accept both orders via deprecation handling (e.g., detect split passed as int/size passed as str).
| from acoupipe.writer import WriteTFRecord, complex_list_feature | ||
|
|
||
| def save_tfrecord(self, features, split, size, name, f=None, num=0, start_idx=0, progress_bar=True): | ||
| def save_tfrecord(self, features, size, name, split='training', f=None, num=0, start_idx=0, progress_bar=True): |
There was a problem hiding this comment.
The public API method signatures were reordered (e.g., generate(features, size, split=...) instead of generate(features, split, size, ...)), which is a breaking change for callers using positional arguments. Suggested fix (mandatory if backward compatibility matters): keep the old positional order and introduce keyword-only parameters for the new preferred order, or accept both orders via deprecation handling (e.g., detect split passed as int/size passed as str).
| DatasetBase.get_output_signature = get_output_signature | ||
|
|
||
| def get_tf_dataset(self, features, split, size, f=None, num=0, start_idx=0, progress_bar=False): | ||
| def get_tf_dataset(self, features, size, split='training', f=None, num=0, start_idx=0, progress_bar=False): |
There was a problem hiding this comment.
The public API method signatures were reordered (e.g., generate(features, size, split=...) instead of generate(features, split, size, ...)), which is a breaking change for callers using positional arguments. Suggested fix (mandatory if backward compatibility matters): keep the old positional order and introduce keyword-only parameters for the new preferred order, or accept both orders via deprecation handling (e.g., detect split passed as int/size passed as str).
| from numpy import concatenate, imag, newaxis, real, searchsorted | ||
|
|
||
|
|
||
| def calc_transfer(ir, fs, blocksize, fftfreq, time_axis=-1): |
There was a problem hiding this comment.
The new calc_transfer() function introduces non-trivial behavior (FFT length selection, zero-padding, bin mapping, and grid validation) but no direct unit tests are added alongside it. Adding targeted tests (e.g., 1D/2D/3D IR shapes, time_axis variations, fftfreq=None, integer-bin vs Hz input, and exact-grid vs off-grid errors) would help prevent regressions and validate the new assumptions.
No description provided.