Skip to content

Conversation

@BioTurboNick
Copy link
Contributor

@BioTurboNick BioTurboNick commented Jan 20, 2025

Profiling code that made heavy use of eigen, creating the final arrays here was a hot spot.

I imagine the @view is going to get a frown... it's strictly reducing the second dimension so in theory it could be resized in-place, but I don't know of a way to do that? EDIT: No longer using a view

const AAA = [0.9124359407098924 0.9654684977248279 0.9237069034235668 0.4479855053248103; 0.6313189619311209 0.039044817699327994 0.21147802367801194 0.2242156559761067; 0.9346485775560693 0.7639560598623737 0.30303677867123346 0.33048614822558353; 0.5034773545892783 0.21614932409092613 0.37837816060665885 0.6122920572414832]

function f(a)
      for i = 1:1000
           eigen(Symmetric(a))
      end
end

Before:
image

After:
image

@andreasnoack
Copy link
Member

It would he good to include the before and after profiling results here to give an idea about the impact

@BioTurboNick
Copy link
Contributor Author

BioTurboNick commented Jan 22, 2025

Thanks, added. Figured I should also do Hamiltonian too because it's the exact same situation.

Looks like there are other spots that could benefit from this as well.

EDIT: found an alternative to @view: initialize Z as a vector and then reshape(resize!()) at the end; extra allocations, but still fewer than the original, and similar performance to the view version.

@dkarrasch
Copy link
Member

This PR is against the release-1.11 branch. Is that on purpose? Otherwise this seems good to go?

@BioTurboNick
Copy link
Contributor Author

Oh, sorry - that's an accident. I relied on the 1.11 branch to make sure the line numbers were right and forked from there to edit. Is it possible to adjust, or do I need to make a new PR?

@dkarrasch
Copy link
Member

I wouldn't know where to change it the target branch. Perhaps you can cherry-pick the commit (after squashing) and open a new PR? Or simply change the few lines manually in a fresh branch?

@jishnub
Copy link
Member

jishnub commented Jan 27, 2025

You may click on the target branch icon on GitHub, which will provide options to change it.

@BioTurboNick BioTurboNick changed the base branch from release-1.11 to master January 27, 2025 02:15
jishnub and others added 8 commits January 27, 2025 10:21
…matrices #54631" to v1.11 (#54742)

(cherry picked from commit b69fc5786331a60c5357bafb901be077e5e3a90d)
…5605)"

This reverts commit 287847936f9c952c6cdbf87fa5ecbbbbd4d1f605.

(cherry picked from commit f09de94afb0c574f84274c746624df2ed840847d)
This backports the following commits:
commit 9690961c426ce2640d7db6c89952e69f87873a93
Author: Jishnu Bhattacharya <[email protected]>
Date:   Mon Apr 29 21:43:31 2024 +0530

    Add upper/lowertriangular functions and use in applytri (#53573)

    We may use the fact that a `Diagonal` is already triangular to avoid
    adding a wrapper.

    Fixes the specific example in
    https://github.com/JuliaLang/julia/issues/53564, although not the
    broader issue. This is because it changes the operation from a
    `UpperTriangular + UpperTriangular` to a `UpperTriangular + Diagonal`,
    which uses broadcasting. The latter operation may also allow one to
    define more efficient methods.

commit 77821cdddb968eeabf31ccb6b214ccf59a604c68
Author: Jishnu Bhattacharya <[email protected]>
Date:   Wed Aug 28 00:53:31 2024 +0530

    Remove Diagonal-triangular specialization

commit 621fb2e739a04207df63857700aca3562b41b5eb
Author: Jishnu Bhattacharya <[email protected]>
Date:   Wed Aug 28 00:50:49 2024 +0530

    Restrict broadcasting to strided-diag Diagonal

commit 58eb2045ddb5dbbfdb759c06239ca54751e73d71
Author: Jishnu Bhattacharya <[email protected]>
Date:   Wed Aug 28 00:44:47 2024 +0530

    Add tests for partly filled parent

commit 5aa6080a580bfbc9453e94a06f3e379e4517b316
Author: Jishnu Bhattacharya <[email protected]>
Date:   Tue Aug 27 20:42:07 2024 +0530

    Reroute Symmetric/Hermitian + Diagonal through triangular

(cherry picked from commit 4ab4722b6ee18009ea7f2fce4a63711a1efcd148)
…6089)

Fix https://github.com/JuliaLang/julia/issues/56085 by calling a newly
created `MulAddMul` object that only wraps the `alpha` (with `beta` set
to `false`). This avoids the explicit multiplication if `alpha` is known
to be `isone`.

(cherry picked from commit 0af99e641a4329b57e48a314e2cedb592e02cd3b)
(cherry picked from commit 415294a560db01f3d89699d8f00bdcba51257e34)
@codecov
Copy link

codecov bot commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.92%. Comparing base (8bb9f6b) to head (39e8882).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1176      +/-   ##
==========================================
+ Coverage   91.89%   91.92%   +0.03%     
==========================================
  Files          34       34              
  Lines       15353    15355       +2     
==========================================
+ Hits        14108    14115       +7     
+ Misses       1245     1240       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dkarrasch dkarrasch merged commit 57e9a0d into JuliaLang:master Jan 27, 2025
4 checks passed
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.

4 participants