Skip to content
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

apply_by_to_key keyword for searchsorted #43509

Conversation

StephenVavasis
Copy link
Contributor

@StephenVavasis StephenVavasis commented Dec 21, 2021

This update to ordering.jl and sort.jl adds a new keyword apply_by_to_key to the searchsorted family of functions and
and insorted. This commit address an issue that trips up many users: the
"by" keyword is unexpectedly applied to the search key. This makes
the functions particularly awkward if the "by" function extracts part
of a larger object. This patch is meant to address issue #9429 as
well as repeats including #33997 and #35418

apply_by_to_key to the searchsorted family of functions and
and insorted.  This commit address an issue that trips up many users: the
"by" keyword is unexpectedly applied to the search key.  This makes
the functions particularly awkward if the "by" function extracts part
of a larger object.  This patch is meant to address issue 9429 as
well as repeats including 33997 and 35418
@jmichel7
Copy link

jmichel7 commented Dec 21, 2021

OK, but why not just apply_by for the keyword? It is clear that the by function is applied in any case to the vector to be searched, so apply_by=false would mean do not apply it to the object being searched.

@DNF2
Copy link

DNF2 commented Dec 21, 2021

Instead of having to use two keywords, one of them really long, how about a different keyword that is only applied to the collection? Maybe in, or something else concise?

(If there's even a single underscore in a name I tend to think there must be better way to phrase it;) in isn't great, I know, it's ok if the function is first or last, maybe, but there must be something concise.

…ned?

This commit defines _Val locally in ordering.jl
to see if this fixes the problem.
@StephenVavasis
Copy link
Contributor Author

I will close this PR myself because I just realized that the core team has already rejected another PR, namely #35418, that also fixes the issue. I would like to urge the core team to find a solution to this problem if 35418 is unsatisfactory.

@jmichel7
Copy link

jmichel7 commented Dec 23, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants