Fix ComputeACG3D soft merge indexing bug that swaps kept units' data - #4776
Open
JESUSROYETH wants to merge 1 commit into
Open
Fix ComputeACG3D soft merge indexing bug that swaps kept units' data#4776JESUSROYETH wants to merge 1 commit into
JESUSROYETH wants to merge 1 commit into
Conversation
Co-authored-by: Arthur031221 <124417490+Arthur031221@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4737.
@Arthur031221's issue already has the full diagnosis and a fix in this same shape, this PR just implements it. I re-verified it against current
mainrather than taking it as given.SortingAnalyzer.merge_units(..., merging_mode="soft")— the default — silently corrupts theacgs_3dextension for kept units whose index shifts during the merge.ComputeACG3D._merge_extension_datacomputesold_unit_ids_indicesagainst the merged sorting, then reuses that same index array to readself.data["acgs_3d"]and["firing_quantiles"]— but those arrays are still laid out in the pre-merge sorting's order. So a kept unit whose index shifted this way comes back holding another unit's 3D-ACG and firing-rate quantiles, with no error and matching shapes, and nothing catches it.ComputeAutoCorrelograms(same module) andComputeUnitLocations(unit_locations.py) already do this correctly: they look upself.sorting_analyzer.sorting.id_to_index(unit_id)on the pre-merge sorting for kept units. This fix makesComputeACG3Ddo the same thing: it reads the source rows from the sorting the data was actually computed against, and writes them to the destination rows of the merged one.The regression test (
test_acgs_3d_merge) is adapted almost verbatim from the issue, and I extended it to also cover sparse analyzers and a multi-segment recording. It fails on unpatchedmainand passes with the fix, across merge groups at the start, middle and end of the unit list, and bothnew_id_strategyvalues.python -m pytest src/spikeinterface/postprocessing -qand.../core -qstill pass otherwise.Two more things from the issue I left untouched here.
test_multi_extensions.py's merge/split check lists 14 extensions andacgs_3disn't one of them, which is likely why this went unnoticed. Also, thebinsvalue this method returns still reflects the pre-merge unit count after a merge (excluded from the new test's comparison, commented why) — a separate, pre-existing bug inComputeACG3D.