Skip to content

Move generate_ground_truth_recording to generation module#4588

Open
h-mayorquin wants to merge 1 commit into
SpikeInterface:mainfrom
h-mayorquin:move-generate-ground-truth-to-generation
Open

Move generate_ground_truth_recording to generation module#4588
h-mayorquin wants to merge 1 commit into
SpikeInterface:mainfrom
h-mayorquin:move-generate-ground-truth-to-generation

Conversation

@h-mayorquin
Copy link
Copy Markdown
Collaborator

This was left out by mistake from #4520. generate_ground_truth_recording should also be in the generation module to avoid a dependency of core into generation.

I also dropped strategy= from test fixtures and the two production callsites that pass it explicitly, keeping the kwarg itself defined on the NoiseGeneratorRecording and MockRecording classes (whose removal is under discussion in #4522). This overlaps with the callsite cleanup in @cwindolf's #4586 (temporally-correlated noise V2, which follows on from #4587) and should make that PR cleaner. Both branches edit the same lines, so the merge should resolve cleanly regardless of order.

@samuelgarcia
Copy link
Copy Markdown
Member

samuelgarcia commented May 20, 2026

Even it looks noisy, I was happy with the explicit strategy= everywhere.
In my mind, it was :

  • generate me a fast random noise, I don't care
  • or generate me a better noise even if it is slower

And more importantly we absolutly need that this must use always 'on_the_fly' !!!

@h-mayorquin
Copy link
Copy Markdown
Collaborator Author

Even it looks noisy, I was happy with the explicit strategy= everywhere. In my mind, it was :

  • generate me a fast random noise, I don't care
  • or generate me a better noise even if it is slower

And more importantly we absolutly need that this must use always 'on_the_fly' !!!

I agree with the on_the_fly. The other strategy does not make sense for the scope of the generate_ground_truth os I don't think it should be there.

@alejoe91 alejoe91 added the generators Related to generator tools label May 20, 2026
@cwindolf
Copy link
Copy Markdown
Collaborator

cwindolf commented May 20, 2026

@samuelgarcia you might want to also check out #4586, where I removed the strategy kw from the NoiseGeneratorRecording as we discussed and only kept the on_the_fly strategy , which I think addresses your comment here? generate_ground_truth would therefore always use on_the_fly, since it uses the NoiseGeneratorRecording

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

Labels

generators Related to generator tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants