Propagate shared annotations in aggregate_channels - #4731
Open
adityasingh2400 wants to merge 1 commit into
Open
Conversation
ChannelsAggregationRecording propagated properties from its child recordings but no annotations at all, so information such as is_filtered was lost and reset to the BaseRecording default. Propagate an annotation when every recording carries it and all of them agree on its value, mirroring the rule already used by UnitsAggregationSorting. Values that cannot be compared safely, such as ragged object arrays, are treated as not shared and dropped rather than raising. Fixes SpikeInterface#3983
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 #3983.
ChannelsAggregationRecordingpropagates properties from its child recordings but propagates no annotations at all, so metadata that every child agrees on is silently lost when you aggregate. The most visible case isis_filtered, whichBaseRecording.__init__sets toFalseon the new aggregate, so aggregating two filtered recordings gave you an aggregate claiming to be unfiltered.This adds annotation propagation just after the existing property block. The rule implemented is that an annotation is propagated only when it is present in every recording in the list and all of the recordings agree on its value. Anything missing from even one recording, and anything where the values differ, is dropped rather than guessed at. This is the same definition of shared that
UnitsAggregationSortinginunitsaggregationsorting.pyalready uses for sortings, so the two aggregation paths now behave consistently.The alternative worth naming is a union rule, where an annotation held by only some recordings is still carried over from whichever ones have it, possibly collected into a per recording list. I did not take that route because the aggregate would then be asserting metadata that is not true of all of its channels, and because it would diverge from the sorting sibling. Happy to switch if you prefer the union semantics.
Two small deliberate differences from the sorting version. First, the equality check uses
np.array_equalrather than building an array of all the values and comparing with==. The sibling idiom is fragile once an annotation value is itself an array, sincenp.arrayon unequal length values either raises or yields an object array, and==on arrays returns an elementwise result instead of a single bool. The comparison is wrapped so that a value which cannot be compared at all, such as a ragged object array, is treated as not shared and skipped rather than crashing the aggregation. Second, the annotation is set withoverwrite=True, which is required becauseBaseRecording.__init__has already setis_filteredby the time this loop runs andset_annotationotherwise raises on an existing key. Sortings do not hit this becauseBaseSorting.__init__sets no default annotations. The value stored is a copy, so the aggregate never shares mutable state with its first child.On
name, which is a main annotation, I chose to include it rather than special case it out. It is only ever present when someone has explicitly set it, sinceBaseExtractor.namefalls back to the class name when the annotation is absent, so this only fires when every child carries the same explicit name. The repr still shows the class alongside it, as inMyRecording (ChannelsAggregationRecording), so the aggregate is not disguised as one of its children, and splitting then re-aggregating now round trips the name instead of dropping it. Excluding it would have been an extra special case that neither the issue nor the sorting sibling asks for, but say the word and I will exclude it.One note on the issue text.
probe_planar_contouris no longer a recording annotation on main, it now lives on theprobeinterface.Probeobject, andset_probegroupno longer writes any annotation, so there is nothing for this change to clobber there. I placed the new block before the probegroup aggregation anyway, and added a test asserting that per probe planar contours and channel locations are unchanged after aggregating recordings that carry probes.Tests added to
test_channelsaggregationrecording.pycover an annotation shared by all children being propagated, one present everywhere with differing values being dropped, one present in only some children being dropped in both list orders, array valued and ragged annotations being handled without raising, the propagated value being a copy rather than a reference into the first child, and the probe metadata regression guard.Reverting the source to main makes three of the new tests fail. The whole file passes after at 19,
test_unitsaggregationsorting.pypasses at 8, and the entirecore/tests/suite runs to completion at 329 passed and 5 skipped.black --line-length 120reports both files unchanged.Disclosure: this change was prepared with AI assistance. I have reviewed and tested it.