Fix: Correct unit ID matching in sortingview curation #2037
Conversation
This file contains 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
Refine the logic for matching unit IDs in the sortingview curation process. Instead of using a potentially ambiguous containment check, unit IDs are now split at the '-' character, ensuring accurate mapping between unit labels and merged unit IDs. Additionally, introduced a unit test to validate the improved behavior and guard against potential false positives in future changes.
for more information, see https://pre-commit.ci
alejoe91
reviewed
Sep 26, 2023
|
Thanks @rkim48 I have one comment. In case of str units, after a merge, the merged unit (e.g. , However, this is not the case for int units. My suggestion would be to convert all unit ids to strings when applying sortingview curation. Note that we have been discussing about forcing unit and channel IDs to always be strings: #1432 |
…iting labels, etc.
for more information, see https://pre-commit.ci
|
Thanks for the feedback. Can you check my latest commit? |
zm711
reviewed
Sep 27, 2023
Remove print('success') statements
Co-authored-by: Zach McKenzie <[email protected]>
for more information, see https://pre-commit.ci
alejoe91
reviewed
Sep 28, 2023
alejoe91
reviewed
Sep 28, 2023
rkim48
commented
Sep 28, 2023
alejoe91
reviewed
Oct 2, 2023
alejoe91
approved these changes
Oct 2, 2023
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.
Issue:
In the process of applying sortingview curation for unit ids of dtype int, the function was using an "in" operator to match unit_id with unit_label, which resulted in false positives. For instance, a unit_id of 10 would mistakenly match a unit_label of 1.
Fix:
Enhanced the unit ID matching logic to handle both integer and string types properly, ensuring correct unit curation.
Added split logic for merged units to correctly identify individual unit parts.
Test introduced:
Introduced a unit test test_false_positive_curation() in test_sortingview_curation.py to capture the false-positive scenario, ensuring unit_id 10 doesn't get labeled when unit_label 1 is specified. It also ensures correct curation for both int and string type unit IDs.
Additional information:
I am using Mountainsort5.