Skip to content

Conversation

@tpapp
Copy link
Owner

@tpapp tpapp commented Dec 8, 2025

No description provided.

Co-authored-by: David Müller-Widmann <devmotion@users.noreply.github.com>
@tpapp
Copy link
Owner Author

tpapp commented Dec 8, 2025

@devmotion: thanks for the suggestion, but I think this is still a bit fragile. If you approve it, I would go with #154.

@devmotion
Copy link
Collaborator

I'd actually prefer this PR, I try to avoid generated functions unless they're the only way to solve the problem. Locally, also on Julia 1.12 this PR seems to fix the problem:

julia> using TransformVariables, StaticArrays

julia> let
           t7 = corr_cholesky_factor(SMatrix{7,7})
           z7 = zeros(dimension(t7))
           @allocations(transform(t7, z7))
       end
0

IMO the only problem with the PR is the test, but in my experience the use of @allocations and @allocated can be quite brittle (and problems seemed to be worse in tests somehow), see also JuliaLang/julia#58780 (comment).

@tpapp
Copy link
Owner Author

tpapp commented Dec 9, 2025

Thanks @devmotion and @mateuszbaran, then I will merge this as is and fix the pipeline to do coverage and allocations tests separately.

@tpapp tpapp merged commit f6bea95 into master Dec 9, 2025
4 of 6 checks passed
@tpapp tpapp deleted the tp/fix-corr-cholesky-allocation3 branch December 9, 2025 08:26
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.

3 participants