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

Fix bug where non-order keyword arguments are passed from sort! to searchsortedfirst #335

Merged
merged 3 commits into from
Feb 22, 2023

Conversation

LilithHafner
Copy link
Member

Silently ignore unrecognized keywords when finding the first index of zero. If stray keywords are passed, they will still cause the inner sort! to throw.

Fixes #334

Backport because I introduced this bug in 1.9 in #303.

@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2023

Codecov Report

Merging #335 (2ff5ff3) into main (ebb87a3) will increase coverage by 0.10%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #335      +/-   ##
==========================================
+ Coverage   93.68%   93.78%   +0.10%     
==========================================
  Files          12       12              
  Lines        7454     7464      +10     
==========================================
+ Hits         6983     7000      +17     
+ Misses        471      464       -7     
Impacted Files Coverage Δ
src/sparsevector.jl 94.75% <100.00%> (+<0.01%) ⬆️
src/SparseArrays.jl 77.77% <0.00%> (-5.56%) ⬇️
src/solvers/spqr.jl 82.16% <0.00%> (-1.09%) ⬇️
src/solvers/cholmod.jl 90.77% <0.00%> (-0.36%) ⬇️
src/solvers/umfpack.jl 88.19% <0.00%> (-0.24%) ⬇️
src/sparsematrix.jl 95.30% <0.00%> (-0.01%) ⬇️
src/abstractsparse.jl 60.41% <0.00%> (ø)
src/linalg.jl 95.87% <0.00%> (+0.21%) ⬆️
src/sparseconvert.jl 92.22% <0.00%> (+6.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Comment on lines +2142 to 2145
searchsortedfirst_discard_keywords(v::AbstractVector, x; lt=isless, by=identity,
rev::Union{Bool,Nothing}=nothing, order::Base.Order.Ordering=Forward, kws...) =
searchsortedfirst(v,x,Base.Order.ord(lt,by,rev,order))
function sort!(x::AbstractCompressedVector; kws...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not apply the same trick to sort! directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want sort!(sprand(10, .1), banana=:blue) to continue to error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. Judging from the code, however, I don't quite see where that call would throw.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should throw at sort!(nz; kws...). I'll also add a test.

@ViralBShah
Copy link
Member

Ok to merge?

@LilithHafner LilithHafner merged commit 38604c3 into main Feb 22, 2023
@LilithHafner LilithHafner deleted the sort-bugfix-334 branch February 22, 2023 03:38
@github-actions
Copy link

The backport to 1.9 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-1.9 1.9
# Navigate to the new working tree
cd .worktrees/backport-1.9
# Create a new branch
git switch --create backport-335-to-1.9
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 38604c3b9f80059f108b196853d2ab9f9956c986
# Push it to GitHub
git push --set-upstream origin backport-335-to-1.9
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.9

Then, create a pull request where the base branch is 1.9 and the compare/head branch is backport-335-to-1.9.

dkarrasch pushed a commit that referenced this pull request Mar 6, 2023
dkarrasch added a commit that referenced this pull request Mar 6, 2023
#303 should've been backported as well, before #335.
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.

SparseArrays sort! function returning kwargs is breaking searchsortedfirst
4 participants