-
Notifications
You must be signed in to change notification settings - Fork 390
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 and refactor partition #2652
Conversation
0da08da
to
6bfafac
Compare
Cmake error in CI seems unrelated.
|
@@ -497,60 +557,20 @@ namespace xt | |||
} | |||
|
|||
template <class E, class C, class = std::enable_if_t<!xtl::is_integral<C>::value, int>> | |||
inline auto partition(const xexpression<E>& e, const C& kth_container, std::ptrdiff_t axis = -1) | |||
inline auto partition(const xexpression<E>& e, C kth_container, std::ptrdiff_t axis = -1) |
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.
This signature is not consistent with the previous overload, where C
is passed by const ref. Passing it by value (to avoid the hidden copy) in the previous overload should be the way to go.
Note: the same should be applied to argpartition
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.
@JohanMabille Actually a copy of the kth
container needs to be made in order to sort it so perhaps better to go with a universal reference.
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.
My point is, if you need a copy of kth_container
in the implementation of partition
, let's pass this argument by value so that the copy is not hidden to the user.
Co-authored-by:JohanMabille
6bfafac
to
2982403
Compare
Checklist
Description
Fix #2651 and refactor (some) of
xsort.hpp
.There was a lot of duplications between different functions in this file.
This is far from what I would like to do, but still a good improvement for now!