search

JuliaManifolds / ManifoldsBase.jl

Basic interface for manifolds in Julia
https://juliamanifolds.github.io/ManifoldsBase.jl/
MIT License
87 stars 8 forks source link

Resolve some basis-related ambiguities #96

Closed mateuszbaran closed 2 years ago

mateuszbaran commented 2 years ago

This resolves some ambiguities detected in Manifolds.jl tests.

mateuszbaran commented 2 years ago

There is also a small issue that some methods of vector_transport_along require c::AbstractVector while other do not. We should probably decide one way or the other.

codecov[bot] commented 2 years ago

Codecov Report

Merging #96 (30cdbb0) into master (845fd77) will decrease coverage by 0.04%. The diff coverage is 100.00%.

:exclamation: Current head 30cdbb0 differs from pull request most recent head 46fd2df. Consider uploading reports for the commit 46fd2df to get more accurate results

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
- Coverage   99.80%   99.75%   -0.05%     
==========================================
  Files          17       17              
  Lines        2041     2041              
==========================================
- Hits         2037     2036       -1     
- Misses          4        5       +1
Impacted Files Coverage Δ
src/DefaultManifold.jl 100.00% <ø> (ø)
src/decorator_trait.jl 100.00% <ø> (ø)
src/parallel_transport.jl 100.00% <100.00%> (ø)
src/point_vector_fallbacks.jl 100.00% <100.00%> (ø)
src/vector_transport.jl 100.00% <100.00%> (ø)
src/ManifoldsBase.jl 97.63% <0.00%> (-1.58%) :arrow_down:
src/ValidationManifold.jl 99.48% <0.00%> (-0.52%) :arrow_down:
src/numbers.jl 100.00% <0.00%> (ø)
src/PowerManifold.jl 99.77% <0.00%> (+0.44%) :arrow_up:

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

kellertuer commented 2 years ago

I think I would opt for putting the AbstractVector, what do you think?

Besides this, we lost a line of code coverage I think

mateuszbaran commented 2 years ago

OK, I'll add AbstractVector then.

I have no idea why we lost coverage though.

kellertuer commented 2 years ago

Uh now we even lost 10 lines, hm. We might have to check that, then; I can try to find time, but only on Friday.

mateuszbaran commented 2 years ago

I'll take a look at it, at least things related to vector transport could be real.

mateuszbaran commented 2 years ago

Hm, we don't have some generic DifferentiatedRetractionVectorTransport methods but no concrete implementation. I'm not sure why it didn't show up earlier in coverage.

mateuszbaran commented 2 years ago

Coverage if full now, I think this can be merged.

kellertuer commented 2 years ago

Looking forward to seeing the effect on Manifolds :)