Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typo in fast templates test #2651

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

alejoe91
Copy link
Member

@alejoe91 alejoe91 commented Apr 2, 2024

@DradeAW there was a typo in the tests, but still fast and normal templates give the same stds down to the 4th decimal (which I think it's ok)

@alejoe91 alejoe91 added bug Something isn't working testing Related to test routines labels Apr 2, 2024
@DradeAW
Copy link
Contributor

DradeAW commented Apr 2, 2024

Yeah, I'm getting -5943.21 sometimes, so I don't think it's a rounding issue ^^

I'll go digging to understand what's going on!

@DradeAW
Copy link
Contributor

DradeAW commented Apr 2, 2024

This is good for me,

Thanks Alessio :)

@@ -864,20 +864,20 @@ def estimate_templates_with_accumulator(

# average
waveforms_sum = np.sum(waveform_accumulator_per_worker, axis=0)
template_means = waveforms_sum
template_means = waveforms_sum.copy()
Copy link
Member

Choose a reason for hiding this comment

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

We need a copy only if computing STD otherwise the copy is useless.

@DradeAW
Copy link
Contributor

DradeAW commented Apr 3, 2024

I don't like this,
Because now, if return_std=False, then waveforms_sum is changed (pointer problem)

Of course it's not used in this case, but I think it's bad practice (what if someone implements something in 8 months from now? This is a potential bug)

@alejoe91
Copy link
Member Author

alejoe91 commented Apr 3, 2024

I don't like this, Because now, if return_std=False, then waveforms_sum is changed (pointer problem)

Of course it's not used in this case, but I think it's bad practice (what if someone implements something in 8 months from now? This is a potential bug)

This was a request from @samuelgarcia, since copying the buffer can be large for many units...

@samuelgarcia
Copy link
Member

memory allocation is not a detail. we should carrefully have this in mind.
averaging should need one allocation and divide in place (like it was the case).
the preprocessing need a big change mainly because of this for instance.

@DradeAW
Copy link
Contributor

DradeAW commented Apr 3, 2024

I agree that memory allocation is not a detail and not something to take likely,

But having two variables (representing completely different things) pointing to the same address seems dubious to me ...

Co-authored-by: Zach McKenzie <92116279+zm711@users.noreply.github.com>
@samuelgarcia samuelgarcia merged commit aca403f into SpikeInterface:main Apr 8, 2024
11 checks passed
@alejoe91 alejoe91 deleted the fix-fast-templates-std branch April 12, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing Related to test routines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants