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

Less allocs in resid2DLinear and faster mean #261

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Conversation

Affie
Copy link
Member

@Affie Affie commented Oct 26, 2023

No description provided.

ret = sum( Lambdas.*dμ )
return ret
# ret = sum( Lambdas.*dμ )
r = map((mu, lam) -> diffop(μ[], mu) * lam, mus, Lambdas)
Copy link
Member Author

Choose a reason for hiding this comment

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

@dehann, here is a version without the extra dispatch and allocations.
Is μ always of length 1?

Copy link
Member

Choose a reason for hiding this comment

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

I think so, will double check bit later today.

Copy link
Member

Choose a reason for hiding this comment

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

looks right

@@ -71,7 +72,7 @@ function _getManifoldFullOrPart(mkd::ManifoldKernelDensity, aspartial::Bool=true
end

function Statistics.mean(mkd::ManifoldKernelDensity, aspartial::Bool=true; kwargs...)
return mean(_getManifoldFullOrPart(mkd,aspartial), getPoints(mkd, aspartial); kwargs...)
return mean(_getManifoldFullOrPart(mkd,aspartial), getPoints(mkd, aspartial), GeodesicInterpolation(); kwargs...)
Copy link
Member Author

Choose a reason for hiding this comment

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

GeodesicInterpolation is currently way faster but still has dynamic dispatch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ExtrinsicEstimation should usually be the fastest way to compute mean. It doesn't work well for some manifolds but for Lie groups it is worth trying out when you prefer speed over accuracy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I'll give it a try. With all the performance enhancements you helped with so far we are seeing a major improvement already.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! Feel free to report any further issues you think I could help with.

Copy link
Member Author

Choose a reason for hiding this comment

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

ExtrinsicEstimation gives an error:

MethodError: no method matching get_embedding(::GroupManifold{ℝ, ProductManifold{ℝ, Tuple{TranslationGroup{ManifoldsBase.TypeParameter{Tuple{2}}, ℝ}, SpecialOrthogonal{ManifoldsBase.TypeParameter{Tuple{2}}}}}, Manifolds.SemidirectProductOperation{RotationAction{LeftAction, TranslationGroup{ManifoldsBase.TypeParameter{Tuple{2}}, ℝ}, SpecialOrthogonal{ManifoldsBase.TypeParameter{Tuple{2}}}}}})

Don't worry about it now though. I think there are other optimizations that will make a bigger difference.

@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2023

Codecov Report

Merging #261 (17bc3a8) into master (6647e8e) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
- Coverage   54.14%   54.09%   -0.06%     
==========================================
  Files          18       18              
  Lines         905      904       -1     
==========================================
- Hits          490      489       -1     
  Misses        415      415              
Files Coverage Δ
src/CommonUtils.jl 62.50% <100.00%> (-0.58%) ⬇️

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

@Affie
Copy link
Member Author

Affie commented Oct 26, 2023

fg = generateGraph_Hexagonal()
@time solveGraph!(fg; multithread=true)
# 2.072770 seconds (16.03 M allocations: 742.617 MiB, 4.83% gc time)

@time solveGraph!(fg)
# 4.217847 seconds (15.65 M allocations: 717.741 MiB, 5.96% gc time)

@Affie Affie requested a review from dehann October 26, 2023 07:39
ret = sum( Lambdas.*dμ )
return ret
# ret = sum( Lambdas.*dμ )
r = map((mu, lam) -> diffop(μ[], mu) * lam, mus, Lambdas)
Copy link
Member

Choose a reason for hiding this comment

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

looks right

@dehann dehann merged commit ee044bd into master Oct 26, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants