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

Improve efficiency of minimum/maximum(::Diagonal) #30236

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

jlapeyre
Copy link
Contributor

@jlapeyre jlapeyre commented Dec 2, 2018

julia> DM = Diagonal(rand(100));

julia> @btime minimum($DM);    # before
  27.987 μs (0 allocations: 0 bytes)

julia> @btime minimum($DM);    # after
  246.091 ns (0 allocations: 0 bytes)

@mcognetta
Copy link
Contributor

Should maximum be included also?

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 2, 2018

Should maximum be included also?

Yes, it should. I'll push another commit to this PR.

I recall checking maximum and finding that there was no performance gain. Puzzling, it didn't make sense. I just tried again now, and indeed the same kind of method yields the same efficiency gain.

@mcognetta
Copy link
Contributor

On this note, these methods can also be sped up for the other diagonal matrix types. In my opinion, one nice way to do it would be to write an iterator that chains the diagonals of the matrices. Then you could just find the min/Max over that iterator. The iterator could be used for some other methods like norm.

I added a method specialized for Diagonal for each of
minimum and maximum.

julia> DM = Diagonal(rand(100));

julia> @Btime minimum($DM);    # before
  27.987 μs (0 allocations: 0 bytes)

julia> @Btime minimum($DM);    # after
  246.091 ns (0 allocations: 0 bytes)
@jlapeyre jlapeyre changed the title Improve efficiency of minimum(::Diagonal) Improve efficiency of minimum/maximum(::Diagonal) Dec 2, 2018
Copy link
Member

@andreasnoack andreasnoack left a comment

Choose a reason for hiding this comment

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

Maybe make it slightly more general with something like

function Base.mapreduce(f, op::Union{typeof(min),typeof(max)}, D::Diagonal{T}) where T
    rdiag = mapreduce(f, op, diag(D))
    return size(D, 1) > 1 ? op(f(zero(T)), rdiag) : rdiag
end

since minimum and maximum will end up calling this method.

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 2, 2018

Maybe make it slightly more general with something like

Yes. I'll look into this.

@cossio
Copy link
Contributor

cossio commented Jun 14, 2020

Bump

@tkf
Copy link
Member

tkf commented Jun 14, 2020

We need to fix #36287 before merging this to ensure isequal(maximum(d), maximum(collect(d))).

@ViralBShah
Copy link
Member

Since #36287 is fixed, should we go ahead and merge this?

@ViralBShah ViralBShah added linear algebra Linear algebra performance Must go faster labels Oct 24, 2023
@andreasnoack andreasnoack merged commit bb138fa into JuliaLang:master Oct 25, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants