Conversation
Signed-off-by: AnneY <[email protected]>
Signed-off-by: AnneY <[email protected]>
Codecov ReportBase: 84.78% // Head: 86.36% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #859 +/- ##
==========================================
+ Coverage 84.78% 86.36% +1.58%
==========================================
Files 138 138
Lines 7117 7116 -1
==========================================
+ Hits 6034 6146 +112
+ Misses 1083 970 -113
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
JohannesMessner
left a comment
There was a problem hiding this comment.
Good catch - just one small performance concern.
| # sort output docs according to input id sorting | ||
| id_to_index = {id_: i for i, id_ in enumerate(ids)} | ||
| return DocumentArray(sorted(docs, key=lambda d: id_to_index[d.id])) | ||
| return DocumentArray([docs[d] for d in ids]) |
There was a problem hiding this comment.
Can we keep the dict-based approach, but have every id_ point to a list of positions, and leverage that in key=... somehow?
The reason it was done this way is performance, we want to gather the id-to-position mapping only once, and then delegate everything else to sorted(), which leverages a fist implementation in C.
There was a problem hiding this comment.
I can't think of a python built-in function. sort doesn't make the list longer than the original one.
There was a problem hiding this comment.
Ok I see, then let's keep it.
Goals:
This PR is related to issue #857