-
Notifications
You must be signed in to change notification settings - Fork 190
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 split in more than 2 units and extend curation docs and tests #2775
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the docstrings @alejoe91. One question.
Co-authored-by: Zach McKenzie <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep looks good with just one more question @alejoe91, and then style-wise we could do
list[str | int] but I don't think that is absolutely necessary for the docstrings, but might make it a bit clearer that ids can still be ints or strings.
Fixes #2816 The split function has been improved to always zero-base the indices list. Now a zero-based array is build out of unique values of the |
for i, split_unit_idx in enumerate(split_unit_indices): | ||
indices_zero_based[segment_index][indices_list[segment_index] == split_unit_idx] = i | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we potentially give i
a better name. In the future I think it will be better to have the counter with the name of what it is rather than just being the counter. To be honest this double for is hard for me to parse because of the i.
Fixes #2694 and replaces #2695 (by @OlivierPeron)
@OlivierPeron when making aPR it's better to create a branch different than main so we can directly contribute to it ;)