Proposal to implement a sorting_analyzer.merge_units() syntax #3043
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
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…o meta_merging_sc2
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
Some questions remain. When we do a merge, do we want to add the possibility to remove spikes that would violate some refractory period? If this, this might make evertyhing a bit more complicated. Also, for some extensions (such as template, unit_locations) I made the choice to use the mean average merges, but this could be discussed. For the template_similarity merge, should we recompute, or just take a sliced value? Minor details that need to be discussed |
|
Perfect thanks @yger. |
the spike_indices was fuzzy between global and local indices and use diffently
for more information, see https://pre-commit.ci
|
Thanks for the tests! Actually I made something very similar, but @samuelgarcia told me not to push anything so I was waiting for his green light.... But it will barely be ready for the release |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
|
@samuelgarcia curation docs updated! Ready to merge on my end |
alejoe91
approved these changes
Jul 15, 2024
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.
This is WIP, and it can be discussed, but the goal of this PR is to implement a sorting_analyzer.merge_units() call that will work as the select_units() one.
Currently, this is only working for some extension, but i'll finish all of them while the PR remains in draft mode. This could be super useful for the GUI and/or for meta merging at the end, since ideally we want to be able to merge units with as few recomputations as possible, given all the precomputed extensions of a sorting_analyzer.
Currently, merges should be given as dict {unit_id : list_unit_ids_to_be_merged}, but this could be disscussed. In this current view, templates are then given as means of merged templates, and i'll do the same for positions. Cross-Corr can only be recomputed for the merged units, ...
In all these functions, no censor period is applied, but after discussing with @alejoe91 it was noted that maybe this would be worth considering. Let's discuss that @samuelgarcia @alejoe91