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

Remove unsafe initialized=true option in sortperm! #47979

Merged
merged 9 commits into from
Dec 31, 2022

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Dec 23, 2022

The reason we had initialized=true as an option was for performance reasons, but the possible performance savings are negligible:

julia> for i in 1:12
           n = 2^i
           v = rand(n)
           ix = collect(eachindex(v))
           total_time = @belapsed sortperm!(copyto!($ix, eachindex($v)), $v, initialized=true) setup=(rand!($v))
           copy_time = @belapsed copyto!($ix, eachindex($v))
           @printf "%4i | %7.2g %7.2g %.1f%%\n" n total_time copy_time copy_time/(total_time-copy_time)*100
       end
 len | sort    init    possible savings from setting `initialized=true`
   2 | 5.3e-07 4.9e-09 0.9%
   4 | 5.4e-07 5.9e-09 1.1%
   8 | 5.7e-07 6.7e-09 1.2%
  16 | 8.1e-07 6.3e-09 0.8%
  32 | 1.2e-06 8.4e-09 0.7%
  64 | 2.1e-06 8.8e-09 0.4%
 128 | 3.9e-06 1.4e-08 0.4%
 256 | 7.8e-06 2.2e-08 0.3%
 512 | 1.6e-05 4.2e-08 0.3%
1024 | 3.3e-05 7.8e-08 0.2%
2048 | 7.1e-05 1.5e-07 0.2%
4096 | 0.00015 3.7e-07 0.2%

This is technically breaking because it changes the behavior of sortperm!([2, 2], [7, 12]; initialized=true) which previously returned [2, 2] and will now return [1, 2].

However, behavior when initialized=true and ix is not a permutation of LinearIndices(v) is currently undefined (explicitly undefined for partialsortperm!, simply not mentioned for sortperm!) and I don't think anyone depends it, so I think this is a valid candidate for the "minor changes" exception.

With this PR we still accept the option for compatibility reasons, but ignore it and don't document it. The current behavior is now the same as prior behavior with initialized=false or unspecified.

Fixes #47977

@LilithHafner LilithHafner added needs pkgeval Tests for all registered packages should be run with this change sorting Put things in order labels Dec 23, 2022
@LilithHafner
Copy link
Member Author

@nanosoldier runtests(ALL)

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@LilithHafner
Copy link
Member Author

@nanosoldier runtests(["StochasticRounding", "TableTransforms", "LicenseGrabber", "GasChromatographySimulator", "FilesystemDatastructures", "Knet", "HarmonicBalance", "NavAbilitySDK", "ProbNumDiffEq", "AlgebraicRL", "PlantBiophysics"])

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here.

@LilithHafner LilithHafner removed the needs pkgeval Tests for all registered packages should be run with this change label Dec 28, 2022
@stevengj
Copy link
Member

This is technically breaking because it changes the behavior

Removing a documented keyword seems like it is more than "technically" breaking.

Maybe we can keep the keyword but just not document it? Or document it as deprecated?

@LilithHafner
Copy link
Member Author

Maybe we can keep the keyword but just not document it?

Agreed. As per the OP:

With this PR we still accept the option for compatibility reasons, but ignore it and don't document it.

Concretely, sortperm!([1, 2, 3], [30, 10, 20]; initialized=true) will still produce [2, 3, 1] without warning or error.

@stevengj
Copy link
Member

Might be good to add a NEWS item?

@LilithHafner LilithHafner added the merge me PR is reviewed. Merge when all tests are passing label Dec 30, 2022
@LilithHafner LilithHafner merged commit 516b097 into master Dec 31, 2022
@LilithHafner LilithHafner deleted the remove-sortperm-initialized branch December 31, 2022 17:57
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sortperm! with initialized=true
4 participants