Skip to content

Move NoiseGeneratorRecording from core to generation#4520

Open
h-mayorquin wants to merge 3 commits intoSpikeInterface:mainfrom
h-mayorquin:refactor_noise_generation
Open

Move NoiseGeneratorRecording from core to generation#4520
h-mayorquin wants to merge 3 commits intoSpikeInterface:mainfrom
h-mayorquin:refactor_noise_generation

Conversation

@h-mayorquin
Copy link
Copy Markdown
Collaborator

@h-mayorquin h-mayorquin commented Apr 16, 2026

Addresses the design discussion in #4482.

NoiseGeneratorRecording mixed two purposes: a lightweight lazy recording for testing and a simulation tool with spatial correlations (cov_matrix) and per-channel noise_levels. This PR introduces a minimal MockRecording in core/generate.py (unit-variance white noise only, no extra parameters) and moves the full NoiseGeneratorRecording to generation/noise_tools.py, where generate_noise() already lived. generate_recording() and generate_recording_by_size() now use MockRecording, while generate_ground_truth_recording() lazy-imports NoiseGeneratorRecording from generation. That way, the work on #4482 can proceed from here without interfering with the testing functionality. I will migrate the tests in a follow-up PR so this diff stays as clear as possible with minimal code churn.

Backward compatibility is preserved via module-level __getattr__ in both core/__init__.py and core/generate.py. Importing NoiseGeneratorRecording from the old paths still works but emits a FutureWarning directing users to import from spikeinterface.generation instead, with removal targeted for 0.106.0. I also changed SilencedPeriodsRecording to compose MockRecording + ScaleRecording instead of importing NoiseGeneratorRecording from core, to comply with a design decision once discussed with @samuelgarcia about the way modules should depend on one another (preprocessing should not depend on generation). I am happy to import from generation directly if you think that is better.

MockRecording still exposes the strategy argument for now. I would like to remove it and keep only tile_pregenerated, but I opened #4522 to discuss whether the simulation side needs on_the_fly for non-repeating noise. I will clean this up in a follow-up once that discussion settles.

@cwindolf
Copy link
Copy Markdown
Collaborator

Hi @h-mayorquin and @samuelgarcia , I'm interested in working on simulated data and would love to see this PR merged so that I can rework my own PRs. @h-mayorquin , from the discussion at #4522, it seems like you plan to remove the strategy argument here, will that be a part of this PR? (I guess it seems like a good time to do it?) And @samuelgarcia , I was wondering if you could help with reviewing to keep this stuff moving?

@cwindolf
Copy link
Copy Markdown
Collaborator

Maybe to keep things moving we could merge this PR as-is and handle the strategy stuff later? I can work on the changes related to the NoiseGeneratorRecording in a follow up PR.

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

I think this is up to @samuelgarcia. Alessio is on vacations now.

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

I think @alejoe91 is back

@alejoe91 @samuelgarcia any blocker on this?

The sampling frequency of the recorder.
durations : list[float]
The durations of each segment in seconds. Note that the length of this list is the number of segments.
noise_levels : float | np.ndarray, default: 1.0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually keep noise levels here. It's useful to have a mock recording test where you can easily modify them. What do you think @h-mayorquin

I agree with rmeoving the cov_matrix!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't think on a use case for that, do you have something specific on mind?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in, in the context of #4482 (comment) I don't see how that would fit on the scope of the mock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants