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

Use GeodesicInterpolationWithinRadius for Circle mean #594

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sethaxen
Copy link
Member

  • Replace default extrinsic mean with interpolation within radius
  • Update docstring
  • Increment patch number

@codecov
Copy link

codecov bot commented Apr 28, 2023

Codecov Report

Merging #594 (3cf54b5) into master (8ccd5d8) will decrease coverage by 92.92%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #594       +/-   ##
==========================================
- Coverage   98.90%   5.98%   -92.92%     
==========================================
  Files          98      98               
  Lines        9897    9701      -196     
==========================================
- Hits         9789     581     -9208     
- Misses        108    9120     +9012     
Impacted Files Coverage Δ
src/manifolds/Circle.jl 0.00% <0.00%> (-100.00%) ⬇️
src/statistics.jl 0.00% <ø> (-98.87%) ⬇️

... and 92 files with indirect coverage changes

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

@sethaxen
Copy link
Member Author

mean doesn't like accepting a vector of numbers and instead wants a vector of vectors (see e.g. https://github.com/JuliaManifolds/Manifolds.jl/actions/runs/4829265624/jobs/8604094066?pr=594#step:5:771). Do we have a standard workaround for this for the Circle?

@mateuszbaran
Copy link
Member

Manopt.jl is currently in the process of adding such workaround: JuliaManifolds/Manopt.jl#236 . Essentially it's wrapping points in one-element vectors. It's not ideal for performance, especially in the case of mean, but it's the easiest solution.

@kellertuer
Copy link
Member

What I basically do is for any p::Number I switch the point to q=[p], then Circle still works fine.
Might be that in PositiveNumbers not all functions work with one-element-vectors yet, will maybe do a PR for that later.

The only thing that is a bit tricky somewhen is that this also requires a small “post-processing” i.e. after computing the (one-vector) mean x you still have to return x[].

@kellertuer
Copy link
Member

HI @sethaxen – do you still plan to continue this? (No pressure, just trying to keep an eye on what PRs we still have open here)

@sethaxen
Copy link
Member Author

sethaxen commented Jan 3, 2024

As if right now, yes, but it's unclear when I will finish it. I have a private lightweight repo efficiently implementing various intrinsic circular statistics that I will eventually finish, and the plan is to then add that as a dependency here to deliver this functionality.

If the plan changes, I'll post here.

@kellertuer
Copy link
Member

That sounds great! Looking forward to hearing from that :)

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