Skip to content

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.

4 participants